-
-
Notifications
You must be signed in to change notification settings - Fork 515
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
feat(biome_js_analyze): noFlatMapIdentity #2324
Conversation
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
CodSpeed Performance ReportMerging #2324 will improve performances by 13.33%Comparing Summary
Benchmarks breakdown
|
crates/biome_js_analyze/tests/specs/nursery/noFlatMapIdentity/invalid.js
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/noFlatMapIdentity/invalid.js
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
i've update the PR |
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/noFlatMapIdentity/valid.js.snap
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
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.
I think we can merge this after fixing the consistency of the wording regarding periods. Thank you for contribution!
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
@isnakode looks good to me, could you execute EDIT: and please write changelog. |
nah, how to write changelog?, any link? |
@isnakode We have the |
https://github.com/biomejs/biome/blob/main/CONTRIBUTING.md There's a whole section about the changelog |
done |
We don't recommend |
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.
LGTM, but we should add more test cases.
crates/biome_js_analyze/tests/specs/nursery/noFlatMapIdentity/invalid.js
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/noFlatMapIdentity/valid.js
Outdated
Show resolved
Hide resolved
markup! { | ||
"flat method can be used to simplify" | ||
}, | ||
)) |
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.
Let's follow the rule pillars:
- show the error
- explain why it could be an error
- suggest a solution (Code action)
crates/biome_js_analyze/src/lint/nursery/no_flat_map_identity.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/noFlatMapIdentity/invalid.js
Outdated
Show resolved
Hide resolved
@isnakode I did some tweak on your branch for it to pass the CI:
Run |
Co-authored-by: unvalley <[email protected]>
Co-authored-by: unvalley <[email protected]>
Co-authored-by: Victorien Elvinger <[email protected]>
|
okay, should i push directly after it, or what? |
Yep, you can push more commits from now on. But the carriage returns may be added back if you run the tests and update the snapshots again. That may need some settings or manual removal. 🤔 |
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.
Thanks for your contribution! You made it!
This will part of the tomorrow's release :)
Summary
Close #2303.
this PR adds lint support to replace unnecessary flatMap call with flat instead, maybe need some help to refine the diagnostic message (my english is so bad)
Test Plan
manually tested