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 lint --skip-formatters option #15427

Merged
merged 1 commit into from
May 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 35 additions & 21 deletions src/python/pants/core/goals/lint.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@
from pants.engine.rules import Get, MultiGet, collect_rules, goal_rule
from pants.engine.target import FieldSet, FilteredTargets
from pants.engine.unions import UnionMembership, union
from pants.option.option_types import IntOption, StrListOption
from pants.option.option_types import BoolOption, IntOption, StrListOption
from pants.util.collections import partition_sequentially
from pants.util.docutil import bin_name
from pants.util.logging import LogLevel
from pants.util.memo import memoized_property
from pants.util.meta import frozen_after_init
from pants.util.strutil import strip_v2_chroot_path
from pants.util.strutil import softwrap, strip_v2_chroot_path

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -193,6 +193,18 @@ def activated(cls, union_membership: UnionMembership) -> bool:
"--only",
help=only_option_help("lint", "linter", "flake8", "shellcheck"),
)
skip_formatters = BoolOption(
"--skip-formatters",
default=False,
help=softwrap(
f"""
If true, skip running all formatters in check-only mode.
Copy link
Member

Choose a reason for hiding this comment

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

Can we just call it "lint mode"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps? I think lint is generally not super well understood and it might be a tautology? I'm thinking check-only mode may be clearer.

Copy link
Member

Choose a reason for hiding this comment

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

My only heartburn comes from the fact that Pants has a check goal. check-if-changed mode?

turbo minor feel free to drop :)


FYI: when running `{bin_name()} fmt lint ::`, there should be little performance
benefit to using this flag. Pants will reuse the results from `fmt` when running `lint`.
"""
),
)
batch_size = IntOption(
"--batch-size",
advanced=True,
Expand Down Expand Up @@ -312,27 +324,29 @@ def batch_by_type(
request_type(batch) for request_type, batch in batch_by_type(lint_target_request_types)
)

batched_fmt_request_pairs = batch_by_type(fmt_target_request_types)
all_fmt_source_batches = await MultiGet(
Get(
SourceFiles,
SourceFilesRequest(
getattr(field_set, "sources", getattr(field_set, "source", None))
for field_set in batch
),
)
for _, batch in batched_fmt_request_pairs
)

fmt_requests = (
request_type(
batch,
snapshot=source_files_snapshot.snapshot,
fmt_requests: Iterable[FmtRequest] = ()
if not lint_subsystem.skip_formatters:
batched_fmt_request_pairs = batch_by_type(fmt_target_request_types)
all_fmt_source_batches = await MultiGet(
Get(
SourceFiles,
SourceFilesRequest(
getattr(field_set, "sources", getattr(field_set, "source", None))
for field_set in batch
),
)
for _, batch in batched_fmt_request_pairs
)
for (request_type, batch), source_files_snapshot in zip(
batched_fmt_request_pairs, all_fmt_source_batches
fmt_requests = (
request_type(
batch,
snapshot=source_files_snapshot.snapshot,
)
for (request_type, batch), source_files_snapshot in zip(
batched_fmt_request_pairs, all_fmt_source_batches
)
)
)

file_requests = (
tuple(request_type(specs_snapshot.snapshot.files) for request_type in file_request_types)
if specs_snapshot.snapshot.files
Expand Down
22 changes: 21 additions & 1 deletion src/python/pants/core/goals/lint_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,12 @@ def run_lint_rule(
rule_runner: RuleRunner,
*,
lint_request_types: Sequence[Type[LintTargetsRequest]],
fmt_request_types: Sequence[Type[FmtRequest]] = [],
fmt_request_types: Sequence[Type[FmtRequest]] = (),
targets: list[Target],
run_files_linter: bool = False,
batch_size: int = 128,
only: list[str] | None = None,
skip_formatters: bool = False,
) -> Tuple[int, str]:
union_membership = UnionMembership(
{
Expand All @@ -174,6 +175,7 @@ def run_lint_rule(
LintSubsystem,
batch_size=batch_size,
only=only or [],
skip_formatters=skip_formatters,
)
specs_snapshot = SpecsSnapshot(rule_runner.make_snapshot_of_empty_files(["f.txt"]))
with mock_console(rule_runner.options_bootstrapper) as (console, stdio_reader):
Expand Down Expand Up @@ -287,6 +289,24 @@ def test_summary(rule_runner: RuleRunner) -> None:
"""
)

exit_code, stderr = run_lint_rule(
rule_runner,
lint_request_types=lint_request_types,
fmt_request_types=fmt_request_types,
targets=targets,
run_files_linter=True,
skip_formatters=True,
)
assert stderr == dedent(
"""\

✕ ConditionallySucceedsLinter failed.
✕ FailingLinter failed.
✓ FilesLinter succeeded.
✓ SuccessfulLinter succeeded.
"""
)


@pytest.mark.parametrize("batch_size", [1, 32, 128, 1024])
def test_batched(rule_runner: RuleRunner, batch_size: int) -> None:
Expand Down