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

feat(biome_glob): add dedicated crate for globs #4609

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Nov 20, 2024

Summary

Move RetsrictedGlob from biome_js_analyze to a dedicated crate.
This allows us to use RetsrictedGlob outside biome_js_analyze.

Also, I renamed RetsrictedGlob into Glob. Possible alternative names are GlobPattern and Pattern.

I added some documentation for the crate and put optional dependencies behind feature flags.

Test Plan

CI must pass.

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Nov 20, 2024
@Conaclos Conaclos requested review from a team November 20, 2024 21:18
@Conaclos Conaclos force-pushed the conaclos/biome_glob branch from 81e938c to 799cd3f Compare November 20, 2024 21:47
Copy link

codspeed-hq bot commented Nov 20, 2024

CodSpeed Performance Report

Merging #4609 will degrade performances by 7.57%

Comparing conaclos/biome_glob (7a64284) with main (516314c)

Summary

❌ 1 regressions
✅ 96 untouched benchmarks

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

Benchmarks breakdown

Benchmark main conaclos/biome_glob Change
pure_9395922602181450299.css[uncached] 4 ms 4.3 ms -7.57%

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Great work @Conaclos! Looking forward to use this crate in other places :)

I left some feedback around docs. Overall, we follow this pattern:

  1. Explain feature
  2. Explain example
  3. Show code example

crates/biome_glob/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_glob/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_glob/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_glob/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_glob/src/lib.rs Show resolved Hide resolved
crates/biome_glob/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_glob/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_glob/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_glob/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_glob/src/lib.rs Show resolved Hide resolved
@cr7pt0gr4ph7
Copy link
Contributor

Just a small nit-pick: RetsrictedGlob should be RestrictedGlob :)

On a (only partially) related note:

  • What's the relationship between biome_service::matcher::Pattern and RestrictedGlob?
  • Is there are reason that prevents us from unifying the two?

@ematipico
Copy link
Member

The idea is to get rid of Pattern. See #4611 for the whole plan, help is very welcome

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Great work @Conaclos !

@Conaclos Conaclos force-pushed the conaclos/biome_glob branch 2 times, most recently from 5f57c0b to 7a64284 Compare November 28, 2024 22:10
@Conaclos Conaclos merged commit 1fbdfdd into main Nov 28, 2024
11 checks passed
@Conaclos Conaclos deleted the conaclos/biome_glob branch November 28, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants