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

📎 CLI flag in linter to write ignores for existing violations #4007

Open
anthonyshew opened this issue Sep 20, 2024 · 5 comments
Open

📎 CLI flag in linter to write ignores for existing violations #4007

anthonyshew opened this issue Sep 20, 2024 · 5 comments
Assignees
Labels
A-CLI Area: CLI S-Feature Status: new feature to implement

Comments

@anthonyshew
Copy link
Contributor

anthonyshew commented Sep 20, 2024

Description

As described in this discussion and in Discord, it would be great to have an option to write in ignores for existing rule violations.

I stumbled upon the need for this feature while working on migrating a large codebase to Biome. There are rules I'd like to activate that have hundreds and thousands of violations, but I can't reasonably fix them all in one PR. I can use some creative override patterning to get it done, but an easier way would be to set ignores for them and follow up with PRs to fix those ignores. There are likely other use cases for this feature, as well.

I will be starting work on this feature momentarily, having received prior blessings from @ematipico and @Conaclos at that Discord pointer.

Open questions:

  • What should we name this? I'm going to use biome lint . --write-suppressions while I work but can name it whatever the crowd prefers.
  • Ignoring for Biome is done with // biome-ignore the/rule/name: <explanation>, so I'd like to allow for an explanation with biome lint . --write-suppressions="TODO: Fix Biome migration ignores.", where my string would become the <explanation>. If not provided, a default explanation will be provided (or we could just let it be <explanation>?). Any thoughts here on what to provide for a default? And does that API sound good?
  • I've only minimally poked around the codebase at this point so codepointers/prior art on how to implement this would be helpful, especially if there is a specific way that sounds preferable to the project maintainers. Otherwise, I'll just have at it. 😄
@dyc3
Copy link
Contributor

dyc3 commented Sep 20, 2024

I personally don't have any problems with the --write-suppressions="TODO: message" flag. IMO, that's about as good as its gonna get. This will definitely be super valuable for migrating. My only suggestion is that there be a default message if none is provided, something to indicate it was automatically generated.

As for code pointers, I don't have perfect context on how suppressions are implemented. But from just a quick look around, I don't think it will be that difficult. You'll probably want to look at:

  • The Rule trait's suppress method, because we already have a LSP code action to suppress lints. (this is probably the most useful one)
  • biome_analyze::handle_comment()
  • the biome_suppression crate (as comment syntaxes are in there)
  • maybe SuppressionAction::find_token_to_apply_suppression()

@ematipico
Copy link
Member

ematipico commented Sep 20, 2024

For the implementation of the feature, I would start simple, and use the default behaviour message of the suppression actions. Then, we can follow up with the customisation of the suppression reason.

The business logic must be implemented at Workspace level, in the function fix_all. This is a function that is implemented for each language, so it's reasonable to send multiple PRs if you think the work will take longer than expected.

You might want to start with javascript.rs, then we can continue with the other languages. There's opportunity to make the code DRY, but don't waste too much time with that unless you really want to. This is where we choose the action to apply to a single violation:

for action in signal.actions() {
// suppression actions should not be part of the fixes (safe or suggested)
if action.is_suppression() {
continue;
}
match params.fix_file_mode {
FixFileMode::SafeFixes => {
if action.applicability == Applicability::MaybeIncorrect {
skipped_suggested_fixes += 1;
}
if action.applicability == Applicability::Always {
errors = errors.saturating_sub(1);
return ControlFlow::Break(action);
}
}
FixFileMode::SafeAndUnsafeFixes => {
if matches!(
action.applicability,
Applicability::Always | Applicability::MaybeIncorrect
) {
errors = errors.saturating_sub(1);
return ControlFlow::Break(action);
}
}
}
}

The analyser infrastructure always provides a suppression action, if implemented. For example, I doubt that GraphQL has that implemented, and JSON doesn't have it.

We will have to update FixAllParams and add a new option, this option will tell to the fix_all action to choose the suppression action. Ideally something like this:

enum ChosenAction {
	/// Chose the code fix
	Default,
	/// Chose the suppression, with an optional reason
	Suppression(Option<String>)
}

@anthonyshew
Copy link
Contributor Author

anthonyshew commented Sep 20, 2024

@ematipico, do you think it makes sense for me to add a FixFileMode so that there's:

pub enum FixFileMode {
    SafeFixes,
    SafeAndUnsafeFixes,
    ApplySuppressions // New
}

My thinking is that --write and --write-suppressions would be mutually exclusive, erroring if you try to supply both. Does that make sense?

@allanlewis
Copy link

FWIW I'd love to be able to apply all fixes (just safe or also unsafe) and suppress the remainder in one command - ruff can't do that.

@anthonyshew
Copy link
Contributor Author

anthonyshew commented Sep 20, 2024

@allanlewis, I'm thinking of this feature as a migration tool rather than a consistent part of your workflow. When that's the case, it seems being able to run both at the same time seems to lose a good bit of value. Also, we're talking about saving milliseconds when we're talking about running one Biome command instead of two...

However, I'm also being selfish. I can see how I can write this if they're mutually exclusive more easily than if they're not, which is why I asked. 😄 If we do think they really need to be ran at the same time, I can sort it out (eventually).

@dyc3 dyc3 added A-CLI Area: CLI S-Enhancement Status: Improve an existing feature S-Feature Status: new feature to implement and removed S-Enhancement Status: Improve an existing feature labels Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI S-Feature Status: new feature to implement
Projects
None yet
Development

No branches or pull requests

4 participants