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

Apply PEP8 to the whole codebase and fix some flake8 errors #1146

Merged
merged 13 commits into from
Nov 11, 2022
Merged

Apply PEP8 to the whole codebase and fix some flake8 errors #1146

merged 13 commits into from
Nov 11, 2022

Conversation

mtazzari
Copy link
Contributor

@mtazzari mtazzari commented Nov 8, 2022 •
edited
Loading

In this PR:

  1. we Fix Make code PEP8 compliant #1132 as we make all the codebase PEP8 compliant, with the following command ran at the repository root:
autopep8 --in-place --recursive --max-line-length 150 --ignore 402 ./

We used with the following customization:

  • max line length is set to 150 (where 79 was the default);
  • we ignore error E402 (docs here) which forces all module imports at top of file. This might clash with some initializations (e.g. the logging module) that we want to perform before importing other modules.
  1. We addressed the following flake8 errors throughout the codebase:
F401: module imported but unused
F522: .format(...) unused named arguments
F524: .format(...) missing argument
F541: f-string without any placeholders

running the following command at the repository root:

flake8  --select F401,F522,F524,F541  oasislmf/
  1. We set up an automated CI workflow called Code Quality that:
  • fails if newly pushed code is not PEP8 compliant (uses autopep8).
  • fails if newly pushed code introduces a flake8 error among those above.

Issue #1147 tracks the job required to address all flake8 errors throughout the codebase, which is likely to require some non-obvious refactoring and thus an accurate integration testing.

Improving Code Quality

This PR makes all the codePEP8 compliant and introduces automatic CI checks to preserve PEP8 compliance.
This PR fixes some flake8 errors and introduces automatic CI checks to avoid that the same errors are introduced in the code in the future.

@mtazzari
Copy link
Contributor Author

mtazzari commented Nov 8, 2022

@sambles have a look at these changes. What do you think?
Here I'm not using the --aggressive flag, and I've allowed lines 150 characters long.

@mtazzari mtazzari mentioned this pull request Nov 9, 2022
Closed
@mtazzari mtazzari added the enhancement New feature or request label Nov 9, 2022
@mtazzari mtazzari self-assigned this Nov 9, 2022
benhayes21
benhayes21 approved these changes Nov 9, 2022
View reviewed changes
Copy link
Contributor

@benhayes21 benhayes21 left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

I've only scanned the changed files quickly but it looks consistent. I find some of the original formatting easier to read, but happy to go with this for enforced consistency.

sambles
sambles reviewed Nov 10, 2022
View reviewed changes
Copy link
Contributor

@sambles sambles 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.

Looks much better without the --aggressive flag

Question on the linting call, using --exit-zero, why not have that fail and cleanup some of the issues it raises too?

I think it makes sense to ignore some of the formatting warnings (since it should be PEP8 anyway), something like flake8 --show-source oasislmf/ --ignore W503,W504.

  • W504 line break after binary operator
  • W503 line break before binary operator
    .. etc with whichever warns we don't care about

@mtazzari
Copy link
Contributor Author

Thank you @sambles I'll have a look now at how we can fix some of the warnings raised by flake8. They seemed quite a few, so I don't know how much time it'd have taken to fix them. But certainly having also flake8 as a 'live' check that makes the workflow fail if it fails is the right direction to go.

@mtazzari
Copy link
Contributor Author

@sambles if you have a look at the failed test Oasislmf Testing / unittest (3.8) (push) you'll notice that most of the failures have to do with the fact that I had to insert #noqa at the end of a very long line, which generates the bash scripts.
Since the test checks the identity of the bash file to a reference file, it fails.

Any ideas how to get around this?
One way would be updating the reference bash scripts, but I feel it is too much a job for this PR, and I find it risky to do, given that code coverage is only 40%.
We could do a separate PR on flake8, which goes into that level of fixing and requires more thorough testing.
Therefore, I'd be inclined to roll back to commit a1fa599
and merge this PR.
What do you think?

@sambles
Copy link
Contributor

sambles commented Nov 10, 2022 •
edited
Loading

@mtazzari

Yup agreed on rolling back and merging this for the PEP8 checking, it makes sense to start a new PR for flake if its a bigger job

We could do a separate PR on flake8, which goes into that level of fixing and requires more thorough testing.
Therefore, I'd be inclined to roll back to commit a1fa599

@mtazzari mtazzari mentioned this pull request Nov 11, 2022
Closed
@mtazzari
Copy link
Contributor Author

@sambles I've reverted back as we discussed. Then, I've just implemented innocuous fixes to flake8 errors:

F401: module imported but unused
F522: .format(...) unused named arguments
F524: .format(...) missing argument
F541: f-string without any placeholders

I've now enforced flake8 to fail the CI workflow if these 4 errors are introduced in new code.
I've opened Issue #1147 to track fixing all flake8 errors.

@sambles sambles self-requested a review November 11, 2022 08:48
sambles
sambles approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@sambles sambles left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the changes

@mtazzari mtazzari linked an issue Nov 11, 2022 that may be closed by this pull request
Make code PEP8 compliant #1132
Closed
@mtazzari mtazzari changed the title Apply PEP8 to the whole codebase Apply PEP8 to the whole codebase and fix some flake8 errors Nov 11, 2022
@mtazzari mtazzari merged commit 9cb38b7 into develop Nov 11, 2022
@mtazzari mtazzari deleted the pep8-codebase branch November 11, 2022 09:59
This was referenced Nov 11, 2022
Closed
Closed
Closed
Closed
sambles pushed a commit that referenced this pull request Jan 12, 2023
@mtazzari @sambles
Apply PEP8 to the whole codebase and fix some flake8 errors (#1146)
14f52b4 
* dummy commit to have the code as in develop in this branch

* pep8 line length 150 non agg

* full PEP8

* [ci] add PEP8 check

* [ci] updated Code Quality checks

* [ci] cleanup

* test non PEP8 code

* [ci] fail if PEP8 doesn't pass

* [ci] remove unnecessary step

* fix non-PEP8 example issue

* revert some changes to cleanup

* [flake8] fix all F401,F522,F524,F524,F541

* [flake8] fix import
@awsbuild awsbuild added this to the 1.27.0 milestone Jan 12, 2023
This was referenced Jan 26, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxwellflitton maxwellflitton maxwellflitton approved these changes

@benhayes21 benhayes21 benhayes21 approved these changes

@sambles sambles sambles approved these changes

@sstruzik sstruzik Awaiting requested review from sstruzik

Assignees

@mtazzari mtazzari

Labels
enhancement New feature or request
Projects
None yet
Milestone
1.27.0
Development

Successfully merging this pull request may close these issues.

Make code PEP8 compliant
5 participants
@mtazzari @sambles @benhayes21 @maxwellflitton @awsbuild