-
Notifications
You must be signed in to change notification settings - Fork 5.8k
[JEWEL-1289] Add Jewel Bugbot review rules #3459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
rock3r
wants to merge
1
commit into
JetBrains:master
Choose a base branch
from
rock3r:sebp/JEWEL-1289_add_bugbot_rules
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| Particular areas to validate when it comes to Jewel code and Jewel-adjacent changes: | ||
| * Is there any breaking change? | ||
| * Binary compat is a non-negotiable. | ||
| * Source compat is always nice to have, but not at the cost of maintaining an infinite amount of overloads. | ||
| * Binary compat can be obtained by leaving the old version of an API as `DeprecationLevel.HIDDEN`. | ||
| * Experimental APIs should never break binary compat unless there is no reasonable exception. | ||
| * Internal APIs offer no stability guarantee whatsoever. | ||
| * Is there any logic issue? | ||
| * If there is any layout logic change in a component, is the layout logic fine? | ||
| * Is it hardcoding things like `fillMaxSize`, which should be left to the user? Components should have a default min size when it makes sense to, and use the `modifier` parameter to be told how big to be by the user. | ||
| * Can you identify if the logic is making any implicit or unreasonable assumption? | ||
| * Is the code changing component styling? | ||
| * If so, does the bridge part of the styling use `JBUI` as source for colors/metrics values? If not, does it at least use LaF keys? | ||
| * Check what the IJPL Swing equivalent components do: do they hardcode values? If they do, we can hardcode those values too, but if they are public constants, we should refer to those instead in the bridge. | ||
| * Hardcoding is only acceptable as a last resort in the bridge; it is tolerated in standalone if a value is not available in the global colors/metrics, or in the palette (we don't have `JBUI` or the same LaF keys, nor IJPL Swing components/UIs available in standalone). | ||
| * Is all the new/changed public API properly documented with KDocs? Is it up to date? Anything missing or unclear? | ||
| * Are we upholding the promise of identical APIs between bridge and standalone (except at the top entry level, e.g. the theme itself)? | ||
| * Are we avoiding leaking IJPL or standalone concerns in "common code"? | ||
| * Do we implement common interfaces properly on both sides? | ||
| * If not, is that properly documented/explained and is there an issue on YouTrack for the follow-up with a corresponding TODO in the code? | ||
| * Validate testing coverage. | ||
| * If there is something we can test with unit tests or Compose UI tests in the PR, is it well covered? Any missing cases? | ||
| * If this is a bug fix, are we adding adequate regression tests? | ||
| * If this is a new component, is its behavior sufficiently covered? | ||
| * Does the PR description use the appropriate release notes template for user-visible changes? | ||
| * "User" in this context means the software engineer using Jewel to build something — not necessarily the end user. | ||
| * See `platform/jewel/docs/pr-guide.md` for the template. | ||
| * Does the release notes section include internal implementation details? They should focus on the user value of a change/fix. | ||
| * API and behavior changes, new functionality, and new features must be covered. | ||
| * Look at `platform/jewel/RELEASE NOTES.md` for a style reference and `platform/jewel/scripts/extract-release-notes.main.kts` for the extraction process. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always good to make sure we point out any behavioral breaking changes in the PR description as well 😎