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

Prefactor lint for upcoming fmt changes #16964

Merged
merged 8 commits into from
Sep 23, 2022

Conversation

thejcannon
Copy link
Member

Prefactoring lint.py in a few ways:

  • Moving the bulk of the logic into a rule_helper which will comprise of most of lint and fmt
  • Removing the ambiguity check. This won't hold ground in the "generic implementation" as we already have one ambiguity (black for files and targets).
  • A few other nits and tweaks

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon added the category:internal CI, fixes for not-yet-released features, etc. label Sep 22, 2022
src/python/pants/core/goals/lint.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/lint.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/lint.py Outdated Show resolved Hide resolved
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@@ -637,7 +643,7 @@ def key(fs: FieldSet) -> str:
return Lint(_get_error_code(all_batch_results))


@rule(level=LogLevel.DEBUG)
@rule(level=LogLevel.TRACE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary to set? It should default to TRACE

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohhh I misunderstood why we were seeing the message. LintResult is a EngineAwareReturnType whose level is INFO.

Hmmm....

Copy link
Member Author

Choose a reason for hiding this comment

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

I've undone the change. If we can stomach seeing the annoying message a bit longer, I have a future change I plan on implementing after the fmt refactor which as I shared with Stu in DM:

My plan to not make LintResult streaming, and use a another rule (like this one) which converts a LintResult into a StreamedLintResult. The difference is right now we force plugin authors to plumb the input snapshot and tool name into LintResult. New change will have that in StreamedLintResult , but not LintResult.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon
Copy link
Member Author

I had to move the batching out of the helper. In fmt we need to batch AFTER creating the "lanes" of files to serially format. Batching before that is meaningless as it'll be undone.

@thejcannon thejcannon merged commit 72e73a3 into pantsbuild:main Sep 23, 2022
@thejcannon thejcannon deleted the lint_prefactor branch September 23, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants