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

Use a linter in workflow to enforce PEP8 compliance #72

Merged
merged 11 commits into from
Sep 5, 2023
Merged

Conversation

sstendahl
Copy link
Collaborator

@sstendahl sstendahl commented Sep 1, 2023

I figured it'd be probably good to use a linter such that the code follows PEP8 standards.
Personally I'm used to flake8, so that's what I set up here. But pylint is an option as well.

The settings for flake8 are set in setup.cfg, and currently look like this:

[flake8]
count = True
ignore = E741, W503, W605
inline-quotes = single

# `make_beam_spectra.py` uses a blank star import (F403), 
# which makes it impossible to recognize certain modules (F405)
per-file-ignores =
	make_beam_spectra.py: F403, F405

From top to bottom:

  • count simply makes it such that flake8 also outputs the total number of linting errors
  • ignore, which PEP8 rules are ignored.
  • inline-quotes: Specifies that single quotes (') are prefered over double quotes ""
  • per-file-ignores: Specific ignore rules that only apply to said file. The reason is explained in the comment above.

Current list of ignored rules, over the defaults of Flake8:
E741: "do not use variables named ‘l’, ‘O’, or ‘I’". The reasoning behind this rule is that l, I and 1 can be difficult to distinguish. Currently there's a case where l is used as variable, and I was not 100% sure what it represented so I kept it that way while reformatting.
W503: Line break before binary operator. This is not an error, having the line break before the binary operator is the requested standard nowadays.
W605: Invalid escape character. The reason is that the escape characters in the LaTeX strings used in the matplotlib labels are recognized as escape character. I'm pretty sure they work though. So this error is given by error.

The code has been reformatted throughout the files to actually pass this test. In most cases this is the addition of whitespace around arithmetic operators, and some reformatting to avoid too long lines. There was also a long list of imports that were unused. Not sure if there's a reason to have them there, but I removed those as well as the linter complained about theses.

Apologies for the long changelist here, there were quite a lot of errors that popped up with things not being PEP-8 compliant.

The Flake8 CI workflow is set such that it "blocks" the PR when it fails.
Fixes issue #34

@sstendahl
Copy link
Collaborator Author

sstendahl commented Sep 1, 2023

Flake8 linting using CircleCI worked, but simply failed when flake8 failed without specifying on which line it failed. This works fine now in the CI workflow I used now with GitHub actions.

There's a bunch of extra plug-ins you can add to flake8 this way (for instance to let it fail on unused arguments, or to make it check import orders etc..), which I think we should do at one point, but that may be better in a different PR as it would require more refactors probably. Currently the only plug in that is added, is to also check that the prefered quote-type is used (single quotes).

@sstendahl sstendahl linked an issue Sep 1, 2023 that may be closed by this pull request
Copy link
Owner

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

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

These look good, and thanks for implementing what is quite a painful (initially) PR. I have no particular requirements to stay with circleCI, so if you want to switch the normal workflow to github then please do so

@sstendahl sstendahl mentioned this pull request Sep 5, 2023
5 tasks
@sstendahl sstendahl merged commit 0e38603 into main Sep 5, 2023
3 checks passed
sstendahl added a commit to sstendahl/HOGBEN that referenced this pull request Sep 6, 2023
Use a linter in workflow to enforce PEP8 compliance
sstendahl added a commit to sstendahl/HOGBEN that referenced this pull request Sep 6, 2023
Use a linter in workflow to enforce PEP8 compliance
@sstendahl sstendahl deleted the linting_test branch October 2, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add linting checks
2 participants