[Python] Fix five classes of PEP-8 violations (E101/E111/E128/E302/W191) #1526
Conversation
|
@modocache Pythonista needed - do you have time to review? That would be greatly appreciated :-) |
|
|
||
| parser.add_argument("--watchos", | ||
| help=""" | ||
| help=""" | ||
| also build for Apple watchos, but disallow tests that require an watchOS device""", |
There was a problem hiding this comment.
Unrelated to this pull request, but we should fix the fact that "watchOS" is spelled two different ways in this single sentence 😂
|
I'm a fan of these changes. 😃 Unrelated, but it would be great if we could make it extremely easy for contributors to the Python in this repository to conform to these style rules. Ideally, I think that would include:
I'd prefer if someone else on the Swift team merged this pull request--I'm a fan of these style changes, but this is a large change to a bunch of code, and I don't want to overstep my bounds. 🙇 |
| swift_repo=args.swift_repo) | ||
| num_samples=args.iterations, verbose=True, | ||
| log_directory=args.output_dir, | ||
| swift_repo=args.swift_repo) |
There was a problem hiding this comment.
This alignment style is not very compatible with 80 columns or 4-space indentation. I would prefer we don't adopt it.
There was a problem hiding this comment.
Alternatively, this could be formatted like so:
run_benchmarks(
file, benchmarks=args.benchmarks,
num_samples=args.iterations,
verbose=True,
log_directory=args.output_dir,
swift_repo=args.swift_repo)I think the rule simply requires that each line of parameters begin at the same indentation. The "ideal" indentation here is subjective, but I think this indentation rule and the 80 columns rule can co-exist.
There was a problem hiding this comment.
That would be a better way to format, indeed.
I think this indentation rule and the 80 columns rule can co-exist.
I can't agree. In the ArgumentParser case below, following this formatting rule pushes the code well beyond 80 columns without a reasonable way to re-format.
|
@gribozavr @modocache Good points. I've now updated this PR to use the formatting suggested by @modocache: run_benchmarks(
file, benchmarks=args.benchmarks,
num_samples=args.iterations,
verbose=True,
log_directory=args.output_dir,
swift_repo=args.swift_repo)I've also fixed the Please let me know if you have any suggestions for further improvements :-) |
|
Thanks, @practicalswift! As for the indentation: I was trying to explain how the rule worked, not necessarily that those lines should be changed. In any case, I'm +1 for this change. :) |
|
@swift-ci please smoke test |
|
@modocache Yes, I know - I applied the general rule as described (not only the specific example) :-) The current PR should not have any of the 80 char issues identified by @gribozavr in the earlier version |
* E101: indentation contains mixed spaces and tabs * E111: indentation is not a multiple of four * E128: continuation line under-indented for visual indent * E302: expected 2 blank lines, found 1 * W191: indentation contains tabs
|
@tkremenek Seems like all tests passed ✅ Let me know if you see the need for further adjustments :-) |
|
@gribozavr + @modocache - do you see the need for any further improvements? :-) |
|
I'm in favor of this change, but will hold off on merging due to @gribozavr's strong opposition in #1526 (comment). |
|
@modocache Oh, I thought that issue had been resolved by the adjustments I made to the PR. Do you or @gribozavr see any 80 column issues with the formatting currently in this PR (after the update)? My ambition was to solve all such issues, but I might have failed in doing so - if so let me know where :-) |
|
How this PR evolved:
some_very_long_function_name(foo,
bar,
baz)
some_very_long_function_name(
foo,
bar,
baz)@gribozavr, are you okay with the latter form? :-) |
|
I don't have concerns with the PR after it was adjusted. |
|
@swift-ci Please test |
[Python] Fix five classes of PEP-8 violations (E101/E111/E128/E302/W191)
|
Thanks as always, @practicalswift! The Python code in this repository owes you a great debt of gratitude! 🙇 |
|
@modocache Thank you for these kind words 😊 |
What's in this pull request?
This commit increases the consistency for the Python code in the repo by fixing all instances of the following five classes of PEP-8 violations:
The corresponding ignore rules are also removed from
.pep8. This will allow for easy identification of potential future regressions with regards to these violations.The Python code in the repo is getting very close to being fully in line with idiomatic Python style as recommended by the Python Software Foundation's Style Guide for Python Code (PEP-8) :-)
Only two violation classes remain after this commit:
Resolved bug number: –
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Note: Only members of the Apple organization can trigger swift-ci.