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: flag to suppress existing diagnostics #4008

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

anthonyshew
Copy link
Contributor

@anthonyshew anthonyshew commented Sep 20, 2024

Summary

Working on #4007 to create a flag for writing ignores.

Test Plan

WIP

@github-actions github-actions bot added the A-CLI Area: CLI label Sep 20, 2024
@anthonyshew
Copy link
Contributor Author

anthonyshew commented Sep 20, 2024

I would appreciate a look from a maintainer here to see if I'm on the right track architecturally. I'm not completely sure that I have the right idea. I was following what looks like the strategy for --fix, which seems like it follows the same workflow (find a diagnostic, make an edit). Plenty more work to do but would love to find out if I'm directionally correct at this point.

Copy link
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

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

Definitely take a look at ematipico's comment here: #4007 (comment)

Comment on lines 223 to 226
// TODO: The flag requires a value here. Am I able to let them pass `--write-suppressions`,
// without a value? Option<Option<String>> wasn't it.
#[bpaf(long("write-suppressions"))]
write_suppressions: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this: pacak/bpaf#383

It seems like some kind of custom Parser is required, but I'd prefer if someone more familiar with bpaf weighed in.

Copy link

@pacak pacak Sep 22, 2024

Choose a reason for hiding this comment

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

If you want to be able to pass both --write-suppressions to get Some("default_val") and --write-suppressions=something to get Some("something") then yea, what you found is the right value.

Make a function fn write_suppressions() -> impl Parser<Option<String>> with code from the last comment (for bar), and do something like

        #[bpaf(external)]
        write_suppressions: Option<String>,

I'd prefer if someone more familiar with bpaf weighed in.

👀

@anthonyshew
Copy link
Contributor Author

anthonyshew commented Sep 20, 2024

TL;DR from Ema's comment: I wasn't even close on my first attempt. 😄 Circling back.

@github-actions github-actions bot added the A-Project Area: project label Sep 22, 2024
@anthonyshew anthonyshew changed the title feat: flag to suppress linting errors feat: flag to suppress existing diagnostics Sep 23, 2024
@github-actions github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Sep 26, 2024
@github-actions github-actions bot removed the A-Formatter Area: formatter label Sep 26, 2024
@github-actions github-actions bot added the A-LSP Area: language server protocol label Sep 26, 2024
@anthonyshew
Copy link
Contributor Author

anthonyshew commented Sep 26, 2024

Well, it at least works now. 😄

Maintainer questions:

  • Would love to hear from a maintainer that I'm architecturally correct now that I've pivoted my strategy
  • Is this the right name for the flag?
  • It looks like I can implement the other languages pretty simply from a quick look? Is this desired?

My to-dos:

  • The default message is still <explanation>
    • Would be great to make the explanation customizable, but will probably do this in a follow up PR
  • Take it for a larger test in a bigger repo than create-turbo
  • Write tests (Not really sure what tests to write)

}

match params.fix_file_mode {
FixFileMode::ApplySuppressions => {
// No-op, handled above
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, this isn't great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-LSP Area: language server protocol A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants