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

Add validate command #480

Merged
merged 13 commits into from
Aug 15, 2024
Merged

Add validate command #480

merged 13 commits into from
Aug 15, 2024

Conversation

surge119
Copy link
Contributor

@surge119 surge119 commented Aug 12, 2024

Summary

Added command to Fixit to check if a config file is valid

Test Plan

Created invalid fixit.toml files such as:

[tool.fixit]
enable = [
    "fixit/rules:DeprecatedABCImport",
]
disable = [
  "fixit.rules",
]

root = true

and ran the new command against them:

# Example from above
$ hatch run fixit validate-config .fixit.toml
Failed to parse rule `fixit/rules:DeprecatedABCImport`: ConfigError: invalid rule name 'fixit/rules:DeprecatedABCImport'

# Another example with invalid TOML
$ hatch run fixit validate-config .
Invalid config: TOMLDecodeError: Expected '=' after a key in a key/value pair (at line 9, column 4)

# pyproject.toml of this project:
$ hatch run fixit validate-config pyproject.toml
Failed to parse rule `.examples.noop`: CollectionError: could not import rule(s) .examples.noop from examples

# Example of config with error and rules with errors:
Failed to parse rule `fixit.rules:DeprecatedABCImport`: NameError: name 'a' is not defined
Failed to parse rule `fixit/rules:DeprecatedABCImport`: ConfigError: invalid rule name 'fixit/rules:DeprecatedABCImport'
Failed to parse rule `fixit.rules`: NameError: name 'a' is not defined

@surge119 surge119 requested review from amyreese and zsol as code owners August 12, 2024 19:51
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 12, 2024
Copy link
Member

@amyreese amyreese 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 I would prefer this to take a direct path to a config (eg, pyproject.toml or fixit.toml), read the "raw" config with read_configs([path]), and then manually call parse_rule() for each string in every enable/disable option (including in every override block). Then call find_rules() on each qualified rule and log errors/warnings for anything that fails to load.

That way, we're not validating the config for a single code path in the repo, but instead validating all possible rules that are referenced from the file, including in all override blocks, to make sure that rules are correctly expressed, findable, and importable.

Bonus points if the warnings/errors can reference the specific block/enable/disable section to make it clear to the user which element is causing the problem.

Copy link
Member

@amyreese amyreese 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 overall. One last request: can we add a simple smoke test that tests this command against a bogus config file and make sure it produces some baseline expected output? You should be able to follow examples in tests/smoke.py for validating expected output on stdout.

src/fixit/cli.py Outdated Show resolved Hide resolved
@amyreese amyreese merged commit f1ea091 into Instagram:main Aug 15, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants