feat: add the checkForEach option to useIterableCallbackReturn#8035
feat: add the checkForEach option to useIterableCallbackReturn#8035skearya wants to merge 1 commit intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 572e679 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request adds a new Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| } | ||
|
|
||
| impl UseIterableCallbackReturnOptions { | ||
| pub const DEFAULT_CHECK_FOR_EACH: bool = true; |
There was a problem hiding this comment.
The default for the source rule is false. Matching the source rule is important because it makes migrating from eslint easier for users.
If we keep the default as true, IMO we can ship this in a patch release and then switch it to default to match the source rule later in a minor release. Or we could switch the default to false and ship this in a minor.
Personally, I'm in favor of the latter. Less work for everybody.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@biomejs/biome": minor | |||
There was a problem hiding this comment.
if this is going to be a minor, the PR must point to next
CodSpeed Performance ReportMerging #8035 will not alter performanceComparing Summary
Footnotes
|
| /// | ||
| /// ## Options | ||
| /// | ||
| /// ### `checkForEach` | ||
| /// | ||
| /// Default: `true` | ||
| /// | ||
| /// When set to `false`, rule will skip reporting `forEach` callbacks that return a value. | ||
| /// | ||
| /// ```json,options | ||
| /// { | ||
| /// "options": { | ||
| /// "checkForEach": true | ||
| /// } | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
Please check the contribution guide for how the format of options should be.
Since the default is true, there's little to no value showing an example with true. Instead, we should have an option block with false, and an example (with or without diagnostics) that shows how false works in that snippet/example.
Even if we change the default value of the option, the suggestion is still valid. We don't want to test the defaults, but the other values
|
Still interested in this pr @skearya ? |
|
I'm happy to pick it up if not, @dyc3 just didn't want to step on toes. These are super easy fixes |
|
Go ahead @theshadow27 Please make sure to cherry pick @skearya commits, so both of you get attributions |
Cool, no problem. See #8289 |
Summary
Adds the "checkForEach" option to the "useIterableCallbackReturn" rule.
When enabled (default behavior) .forEach() callbacks will be checked to not return anything. When disabled the callbacks will not be checked for behavior. This option exists in the eslint rule.
Rule Option:
{ "options": { "checkForEach": true } }Closes #8024
Relevant:
#8005
Test Plan
Docs