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

Apply natural import order for JS to synchronise the code base with Biome and its configuration #2423

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

chadyred
Copy link
Contributor

@chadyred chadyred commented Dec 4, 2024

Q A
Bug fix? no
New feature? no
License MIT

Hello,

Biome sounds really good 🧑‍🔬 , it seems to be a really cool tool, guys !

It applies a rule for ordering import "naturally" (https://biomejs.dev/analyzer/import-sorting/) so, I apply the command to check and fix

What about apply it "as is" ? Another alternative will be to disabled the rule. What do you think about that detail ?

Maybe, an import needs of a package just above, from another package import, this is what I propose also to disable the rule, WYT ?

Thanks ^^

@chadyred chadyred requested a review from Kocal as a code owner December 4, 2024 11:04
@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Dec 4, 2024
@chadyred chadyred changed the title chore: Apply natural import order for JS used by biome Apply natural import order for JS used by biome Dec 4, 2024
Copy link

github-actions bot commented Dec 4, 2024

📊 Packages dist files size difference

ℹ️ No difference in dist packagesFiles.

@chadyred chadyred force-pushed the chore/apply-nartural-import-order branch 3 times, most recently from ebe55c7 to d00d876 Compare December 4, 2024 13:47
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Seems nice, but by curiosity I don't see any changes about Biome.js changes, did you forgot to commit something? :)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Dec 4, 2024
@chadyred
Copy link
Contributor Author

chadyred commented Dec 5, 2024

Hey @Kocal,

I didn't edit the conf (I play with it on my side :p )

I exposed the fact I discovered the tool, I follow the contributing guideline, and I thinked about the rule to sort import has not been discussed / seen / treated before. So I pointed out the fact if we apply to the code base a global check then fix with actual rules of Biome config, we will apply (like in this PR) a lot of changed. If we keep Biome and its conf as is, ok, the PR is good, and the code base is synchronized with Biome conf...that's it!

@chadyred chadyred force-pushed the chore/apply-nartural-import-order branch from d4f7315 to 1ae34f2 Compare December 5, 2024 08:07
@chadyred chadyred changed the title Apply natural import order for JS used by biome Apply natural import order for JS to synchronise the code base with Biome and its configuration Dec 5, 2024
@chadyred chadyred force-pushed the chore/apply-nartural-import-order branch 5 times, most recently from 60c291a to 89d499f Compare December 5, 2024 08:54
@chadyred chadyred force-pushed the chore/apply-nartural-import-order branch from 89d499f to 331a409 Compare December 5, 2024 09:01
@Kocal
Copy link
Member

Kocal commented Dec 5, 2024

Yeah configuring how imports are sorted is nice, not doing it is sometimes a waste of time in some projects/teams when reviewing the code, but AFAIK it didn't happen in Symfony UX.

Can you please commit the new Biome.js configuration, so we can merge your PR? :)

Thanks!

Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Putting a guard, waiting for Biome.js configuration to be updated 🙏🏻

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Reviewed Has been reviewed by a maintainer labels Dec 5, 2024
@chadyred
Copy link
Contributor Author

chadyred commented Dec 5, 2024

@Kocal I did not change it, it is the default config, I just callnode_modules/.bin/biome check --fix simply with the existing conf (https://biomejs.dev/analyzer/import-sorting/ : it is enabled by default, it is implicite)

@Kocal
Copy link
Member

Kocal commented Dec 5, 2024

@chadyred Ah!

This feature is enabled by default but can be opted-out via configuration:

Alright, I thought it was the opposite, and expected to see Biome configuration (or commands) to be updated to enable imports sortings.

However, you used check command, and given https://biomejs.dev/reference/cli/ it looks like only biome check and biome ci commands sort imports 🤔
It's a bit unfortunate because we use biome lint and biome format commands, for... historical reason (ESLint and Prettier).

You know, let's iterate, I'm merging this and will open a new PR for biome check and biome ci.

Thanks @chadyred :)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Work Additional work is needed labels Dec 5, 2024
@Kocal
Copy link
Member

Kocal commented Dec 5, 2024

Thank you @chadyred.

@Kocal Kocal merged commit 4d298e8 into symfony:2.x Dec 5, 2024
64 checks passed
@chadyred
Copy link
Contributor Author

chadyred commented Dec 5, 2024

You are welcome, thanks for your time @Kocal 🍏

Kocal added a commit that referenced this pull request Dec 5, 2024
…mands, replace them with check and ci commands (Kocal)

This PR was merged into the 2.x branch.

Discussion
----------

[Meta] Drop format, lint, check-format and check-lint commands, replace them with check and ci commands

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

Following #2423 (comment).

When I've migrated from ESLint and Prettier to Biome.js, I kept commands `yarn lint`, `yarn format` (and `yarn check-`) for historical and simplicity reasons, because it was two separate tools.

_Now_ that we use a single tool, we can use its two dedicated commands:
- [`biome check`](https://biomejs.dev/reference/cli/#biome-check), like `biome format + biome lint + imports sorting`
- [`biome ci`](https://biomejs.dev/reference/cli/#biome-ci), for the CI (it does not write files)

<img width="1116" alt="image" src="https://github.com/user-attachments/assets/ea3481f8-308a-4a8a-a47f-53692c0a3488">

cc `@chadyred`

Commits
-------

36d04ff [Meta] Drop format, lint, check-format and check-lint commands, replace them with check and ci commands
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants