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.

Sign up for GitHub

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

Jump to bottom

feat(lint): add support for config file and CLI flags for rules #11776

Merged
merged 20 commits into from
Sep 3, 2021
Merged

feat(lint): add support for config file and CLI flags for rules #11776

merged 20 commits into from
Sep 3, 2021

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Aug 19, 2021 •
edited
Loading

This commit adds support for following flags in deno lint subcommand:

  • --config - allows to load configuration file and parses "lint" object
  • --rules-tags=<tags> - allows specifying which set of tagged rules should be run
  • --rules-include=<rules> - allow specifying which rules should be run
  • --rules-exclude=<rules> - allow specifying which rules should not be run

Ref #11686

bartlomieju
bartlomieju commented Aug 19, 2021
View reviewed changes
cli/config_file.rs Outdated Show resolved Hide resolved
@bartlomieju bartlomieju added this to the 1.14.0 milestone Aug 19, 2021
bartlomieju
bartlomieju commented Aug 20, 2021
View reviewed changes
cli/tools/lint.rs Outdated Show resolved Hide resolved
lucacasonato
lucacasonato reviewed Aug 24, 2021
View reviewed changes
cli/config_file.rs Outdated Show resolved Hide resolved
This was referenced Aug 24, 2021
Closed
Open
dsherret
dsherret reviewed Aug 25, 2021
View reviewed changes
cli/config_file.rs Outdated Show resolved Hide resolved
cli/tests/integration/lint_tests.rs Show resolved Hide resolved
cli/tests/testdata/lint/Deno.jsonc Show resolved Hide resolved
cli/tools/lint.rs Outdated Show resolved Hide resolved
cli/tools/lint.rs Show resolved Hide resolved
lucacasonato
lucacasonato requested changes Aug 25, 2021
View reviewed changes
Copy link
Member

@lucacasonato lucacasonato left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are no flags for lint.rules.tags, lint.rules.include, and lint.rules.exclude.

cli/config_file.rs Outdated Show resolved Hide resolved
cli/config_file.rs Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

There are no flags for lint.rules.tags, lint.rules.include, and lint.rules.exclude.

Correct. What's your suggestion here?

@lucacasonato
Copy link
Member

Correct. What's your suggestion here?

We agreed on no config file options that don't also have flags.

@bartlomieju
Copy link
Member Author

Correct. What's your suggestion here?

We agreed on no config file options that don't also have flags.

I can add the flags. Honestly having only files configuration is useless in my opinion.

dsherret
dsherret reviewed Aug 25, 2021
View reviewed changes
cli/tools/lint.rs Outdated Show resolved Hide resolved
@bartlomieju bartlomieju mentioned this pull request Aug 27, 2021
Open
@bartlomieju bartlomieju changed the title feat(lint): add support for config file feat(lint): add support for config file and CLI flags for rules Aug 31, 2021
@bartlomieju
Copy link
Member Author

This PR is now feature complete, but I'm very unhappy about the code quality and intend to give it another pass to refactor it and make code better.

I also want to add some more integration tests to test all combinations of config/flags passed to "deno lint".

magurotuna
magurotuna reviewed Sep 1, 2021
View reviewed changes
cli/config_file.rs Outdated Show resolved Hide resolved
cli/tools/lint.rs Show resolved Hide resolved
This was referenced Sep 1, 2021
Closed
Closed
Merged
dsherret
dsherret approved these changes Sep 3, 2021
View reviewed changes
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

lucacasonato
lucacasonato approved these changes Sep 3, 2021
View reviewed changes
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Looks great. Well done @bartlomieju.

@bartlomieju bartlomieju merged commit d93570a into denoland:main Sep 3, 2021
@bartlomieju bartlomieju deleted the lint_config branch September 3, 2021 15:02
@bartlomieju bartlomieju mentioned this pull request Sep 7, 2021
Closed
@bartlomieju bartlomieju mentioned this pull request Sep 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grian32 grian32 grian32 left review comments

@magurotuna magurotuna magurotuna left review comments

@caspervonb caspervonb caspervonb left review comments

@lucacasonato lucacasonato lucacasonato approved these changes

@dsherret dsherret dsherret approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.
6 participants
@bartlomieju @lucacasonato @caspervonb @dsherret @magurotuna @grian32