-
-
Notifications
You must be signed in to change notification settings - Fork 508
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(cli): add --write
, --fix
and --unsafe
options to fixable commands
#2898
Conversation
Deprecate --apply and --apply-unsafe
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 added test cases for snapshots, but they might not be very valuable (some cases should be tested). Although the commands are different, the actual behavior is the same, so unit tests might be sufficient. If they seem unnecessary, I'll remove them.
@@ -548,7 +548,7 @@ pub struct FormatOnTypeParams { | |||
pub offset: TextSize, | |||
} | |||
|
|||
#[derive(Debug, Clone, Copy, serde::Serialize, serde::Deserialize)] | |||
#[derive(Debug, Clone, Copy, serde::Serialize, serde::Deserialize, PartialEq)] |
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.
PartialEq
is needed in unit tests to check incompatible command tests.
crates/biome_cli/src/commands/mod.rs
Outdated
#[bpaf(long("apply"), switch, hide_usage)] | ||
apply: bool, | ||
/// Apply safe fixes and unsafe fixes, formatting and import sorting | ||
#[bpaf(long("apply-unsafe"), switch)] | ||
#[bpaf(long("apply-unsafe"), switch, hide_usage)] |
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.
note; Hide --apply
, --apply-unsafe
, please check snapshot diff for the help command.
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.
As a preliminary comment, the command biome migrate
also accepts the --write
option. This means it should accept --fix
too
fix, | ||
unsafe_, | ||
} = options; | ||
if apply && apply_unsafe { |
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.
We might want to print a warning in the console, and communicate that --apply
and --apply-unsafe
are deprecated, and users should use --fix
/--write
and --unsafe
If we're worried about snapshots, we could use
if cfg_if!(debug_assertion) {}
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.
It seems that we don't have cfg_if crate in biome_cli, I have a PR that deprecates --apply
and --apply-unsafe
unvalley#3
I think diffs due to deprecation (--apply
, --apply-unsafe
and --help
, etc.) should be recorded in the snapshot, so I would like to address that in another PR. Or, if it is not a problem, I will include the deprecation change in this PR.
@unvalley this is a reminder for you: we'll have to update the website to use these new options instead of the old one. Also, the changelog doesn't say anything about the deprecation of the old CLI options |
@ematipico As for deprecation, I created a new PR #2918 I'll close the website update issue biomejs/website#398 |
Summary
Closes #2267
Add
--write
,--fix
and--unsafe
options tobiome check
andbiome lint
commands.--apply
->--fix
or--write
--apply-unsafe
->--fix --unsafe
or--write --unsafe
Add
--fix
option tobiome format
andbiome migrate
commands.--unsafe
should be used with--fix
or--write
.--apply
and--apply-unsafe
cannot be used with any--fix
,--write
,--unsafe
--unsafe --fix
and--unsafe --write
is allowed, iow it doesn't care the orderTest Plan
remained
--apply
and--apply-unsafe
, it will be removed in biome v2.0