Skip to content
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

chore: to_lowercase -> to_lowercase_cow #4030

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

minht11
Copy link
Contributor

@minht11 minht11 commented Sep 21, 2024

Summary

Follow up to #4014
Implement to_lowercase_cow and use it instead to_lowercase. Enforce future usage with clippy rule.
There are still some to_lowercase left which are trickier to migrate, those and the clippy enforcement will be done with separate PR.

Test Plan

All tests should pass.

@minht11 minht11 self-assigned this Sep 21, 2024
@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS labels Sep 21, 2024
Copy link
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to add update the clippy lint in clippy.toml too

@minht11
Copy link
Contributor Author

minht11 commented Sep 21, 2024

@dyc3 There are still places which are cannot easily migrated to Cow version. For now I just want to see if there is any perf improvement.
I will likely finish migrating/enforcing with separate PR.

Comment on lines 465 to 467
fn to_lowercase_cow(&self) -> Cow<Self> {
panic!("OsStr is not supported")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what todo here. OsStr does not support to_lowercase. Implementing it manually is hard and we don't really need it.

Is leaving as is fine? Do we need separate trait?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose a separate trait would be safest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split it into two separate traits, shame trait-alias are not stable so only one trait could be imported for both. We could use trait-set macro but I imagine introducing new crate for this would be overkill.

Copy link

codspeed-hq bot commented Sep 21, 2024

CodSpeed Performance Report

Merging #4030 will degrade performances by 7.41%

Comparing minht11:chore/replace-to-lowercase-with-cow (fdec823) with main (d20ad1e)

Summary

❌ 1 regressions
✅ 106 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main minht11:chore/replace-to-lowercase-with-cow Change
router_17129688031671448157.ts[cached] 2 ms 2.2 ms -7.41%

@github-actions github-actions bot added A-Formatter Area: formatter A-Tooling Area: internal tools labels Sep 21, 2024
@minht11 minht11 force-pushed the chore/replace-to-lowercase-with-cow branch from 7695433 to df730d4 Compare September 21, 2024 18:29
@github-actions github-actions bot added A-Project Area: project A-Parser Area: parser L-JSON Language: JSON and super languages L-HTML Language: HTML labels Sep 21, 2024
@minht11 minht11 marked this pull request as ready for review September 21, 2024 20:27
@minht11 minht11 requested review from a team September 21, 2024 20:28
@github-actions github-actions bot added the L-Grit Language: GritQL label Sep 26, 2024
@minht11 minht11 merged commit 75b4387 into biomejs:main Sep 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter A-Linter Area: linter A-Parser Area: parser A-Project Area: project A-Tooling Area: internal tools L-CSS Language: CSS L-Grit Language: GritQL L-HTML Language: HTML L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants