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: create "biome" hook, preserve "rome" hook as alias #432

Merged
merged 7 commits into from
Apr 24, 2024

Conversation

oxcabe
Copy link
Contributor

@oxcabe oxcabe commented Apr 13, 2024

This PR does the following:

  • Renames the "rome" hook to "biome", correcting any references to the rome tool.
  • Sets the "rome" hook as an alias to the "biome" hook as a backwards compatibility guard.
  • Adds relevant documentation to the README.

Fixes #428.

@oxcabe oxcabe marked this pull request as ready for review April 13, 2024 10:57
Copy link
Member

@sandydoo sandydoo left a comment

Choose a reason for hiding this comment

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

@oxcabe, could you please remove all the code formatting changes? The diff should be readable and contain just the changes you're proposing. Thanks!

@oxcabe
Copy link
Contributor Author

oxcabe commented Apr 16, 2024

Sure thing, I'm not even sure when the files got formatted 😅

@oxcabe oxcabe requested a review from sandydoo April 16, 2024 10:03
Copy link
Member

@sandydoo sandydoo left a comment

Choose a reason for hiding this comment

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

I think the better approach here would be to use mkRenamedOptionModule to rename rome -> biome. You get the alias and people will be prompted to switch.

If you feel like an alias (with no warning) is more appropriate for now, then you can use mkAliasOptionModule instead.

There's no need to have both rome and biome in config.hooks.

@oxcabe oxcabe force-pushed the fix/biome-rome-confusion branch 6 times, most recently from c0b916f to 8b255d0 Compare April 16, 2024 19:04
@oxcabe
Copy link
Contributor Author

oxcabe commented Apr 17, 2024

I know it's failing as of my last commit. Currently trying to figure out by myself what am I doing wrong and why 👍🏼

@oxcabe
Copy link
Contributor Author

oxcabe commented Apr 17, 2024

I think the better approach here would be to use mkRenamedOptionModule to rename rome -> biome. You get the alias and people will be prompted to switch.

Implemented this approach. For some reason, the deprecation warning is not appearing when enabling the hook:

rome.enable = true;

There's no need to have both rome and biome in config.hooks.

I can't seem to make it work without having config.hooks entries for both tools. By omitting rome there, the following error appears:

error: The option `hooks.rome.entry' is used but not defined.

@oxcabe oxcabe requested a review from sandydoo April 17, 2024 11:25
modules/hooks.nix Show resolved Hide resolved
Co-authored-by: sander <sandermel94@gmail.com>
@oxcabe
Copy link
Contributor Author

oxcabe commented Apr 20, 2024

From the failing build:

pre-commit-run> - hook id: nixpkgs-fmt
pre-commit-run> - files were modified by this hook

@oxcabe, could you please remove all the code formatting changes? The diff should be readable and contain just the changes you're proposing. Thanks!

What should I do here?

@oxcabe oxcabe requested a review from sandydoo April 20, 2024 14:38
@sandydoo
Copy link
Member

See if you can move that rec around to stop it from reformatting everything. If not, then commit the formatting changes.

@oxcabe
Copy link
Contributor Author

oxcabe commented Apr 20, 2024

Ready!

@domenkozar domenkozar merged commit 6fb82e4 into cachix:master Apr 24, 2024
4 checks passed
@oxcabe oxcabe deleted the fix/biome-rome-confusion branch April 24, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

biome is being used instead of rome
3 participants