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

Update project to use ruff for formatting and linting #81

Merged
merged 9 commits into from
Mar 6, 2024
Merged

Update project to use ruff for formatting and linting #81

merged 9 commits into from
Mar 6, 2024

Conversation

jatkinson1000
Copy link
Owner

@jatkinson1000 jatkinson1000 commented Mar 4, 2024 •
edited
Loading

Closes #77

Update formatting and linting to use ruff.

This is based on top of handicaps-refactor branch in anticipation of #78 being merged soon.

It took me a while to get my head around ruff and play with it.
I like how configurable it is, but the fact that rules from an existing package (i.e. pylint) may be spread between other rulesets to avoid duplicates makes it tricky to get the same behaviour/how you want it.
I added some rules/rulesets that seemed sensible but didn't trigger anything - it is possible something will trigger in future with code that that feels sensible, so I am open to adding exceptions with good reason.

  • custom config for this project written in pyproject.toml
    • Apply pylint to match previous linting
    • Apply a bunch of flake-8 to catch pylint rules not covered by the pylint ruff extension
    • Apply isort
    • Apply pydocstyle with numpy
    • Apply whitespace linting
    • Apply ruff rules because why not
    • Ignore if/else -> ternary and ambiguous characters
  • Applied rules to code
  • Updated workflows to use ruff and removed old tools
  • Added blackdoc as ruff does not apply to .rst files

Notes:

  • Duplicate code warnings no longer appear, perhaps will in a future update?

  • ruff does not work on non-python files so needed to add blackdoc for docs.

  • Source code updated to address issue

  • Style and formatting applied

  • Tests written to cover changes

  • Docstrings included/updated in code

  • Project documentation updated as necessary

@jatkinson1000 jatkinson1000 added documentation Improvements or additions to documentation enhancement New feature or request labels Mar 4, 2024
@jatkinson1000 jatkinson1000 self-assigned this Mar 4, 2024
TomHall2020
TomHall2020 suggested changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@TomHall2020 TomHall2020 left a comment

Choose a reason for hiding this comment

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

Raising errors with tuples instead of message strings needs fixing, everything else is style or general comments.

archeryutils/classifications/agb_field_classifications.py Outdated Show resolved Hide resolved
archeryutils/classifications/agb_indoor_classifications.py Show resolved Hide resolved
archeryutils/classifications/agb_indoor_classifications.py Outdated Show resolved Hide resolved
archeryutils/classifications/agb_indoor_classifications.py Outdated Show resolved Hide resolved
archeryutils/classifications/agb_outdoor_classifications.py Show resolved Hide resolved
archeryutils/classifications/classification_utils.py Outdated Show resolved Hide resolved
archeryutils/tests/test_rounds.py Outdated Show resolved Hide resolved
@jatkinson1000 jatkinson1000 force-pushed the ruff branch 4 times, most recently from 8913f01 to 7b173aa Compare March 5, 2024 19:39
@jatkinson1000
Copy link
Owner Author

@TomHall2020 I have been a bit naughty with git magic, but the tuple error messages should be gone now.
Still not entirely sure how they got in... I think I was using an older version of ruff at one point that had this as a bug (and the new version can't spot it). The ruff installed by my system package manager was overriding the one in my virtual environment. -_-

@jatkinson1000
Update pyproject.toml with a ruff config.
20ee930
@jatkinson1000
Apply ruff config to archeryutils code for both linting and formatting.
d14101c
@jatkinson1000
Update linting workflow to use ruff.
b6866e3
@jatkinson1000
Update pyproject.toml to remove old linting tools and update docs to …
09cc019 
…replace old linting tools with ruff.
@jatkinson1000
Add blackdoc for checking docs as ruff does not apply to .rst files. …
b54175f 
…Also includes blackdoc check of docs in workflow.
@jatkinson1000
Update README to remove black and pylint badges and add ruff.
421931c
@jatkinson1000
Disable magic number comparisons for tests in ruff config.
c42355d
@jatkinson1000
Restructure classification assignment to be cleaner and bugfix issue …
c1b2c31 
…causing tests to fail as a result of infinite loop due to next(iter()) inside a try-except.
@jatkinson1000
Improve classification utils to be clearer about age group comparison…
b427427 
…s and remove magic number.
@codecov Codecov
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 89.00000% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 97.27%. Comparing base (58c013c) to head (f54f469).

❗ Current head f54f469 differs from pull request most recent head b427427. Consider uploading reports for the commit b427427 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
- Coverage   97.86%   97.27%   -0.59%     
==========================================
  Files          28       28              
  Lines        1499     1505       +6     
==========================================
- Hits         1467     1464       -3     
- Misses         32       41       +9
Files Coverage Δ
archeryutils/classifications/__init__.py 100.00% <100.00%> (ø)
...utils/classifications/agb_field_classifications.py 100.00% <100.00%> (ø)
...tils/classifications/agb_indoor_classifications.py 100.00% <100.00%> (ø)
.../classifications/agb_old_indoor_classifications.py 100.00% <100.00%> (ø)
...ils/classifications/agb_outdoor_classifications.py 100.00% <100.00%> (ø)
...cheryutils/classifications/tests/test_agb_field.py 100.00% <100.00%> (ø)
...heryutils/classifications/tests/test_agb_indoor.py 100.00% <100.00%> (ø)
...utils/classifications/tests/test_agb_old_indoor.py 100.00% <100.00%> (ø)
...eryutils/classifications/tests/test_agb_outdoor.py 100.00% <100.00%> (ø)
...classifications/tests/test_classification_utils.py 100.00% <ø> (ø)
... and 17 more

@jatkinson1000
Copy link
Owner Author

@TomHall2020 Rebased on top of main after merging your #76 and my #80 if you are now happy with this going in?

@TomHall2020
Copy link
Contributor

@TomHall2020 Rebased on top of main after merging your #76 and my #80 if you are now happy with this going in?

Despite the pain of thinking about rebasing #59 yet again, yeah this looks good, and I definetly won't miss waiting 10 seconds for pylint to run before each commit in the future

@jatkinson1000
Copy link
Owner Author

Despite the pain of thinking about rebasing #59 yet again, yeah this looks good, and I definitely won't miss waiting 10 seconds for pylint to run before each commit in the future

Yeah, sorry, hopefully it isn't too painful...
I think I'd prefer it this way round as linting commits are 'bad' since they touch lots of files, but not in a very useful way when trying to look back over git history to see what changed when and why.

@jatkinson1000 jatkinson1000 merged commit 8f45558 into main Mar 6, 2024
10 checks passed
@jatkinson1000 jatkinson1000 deleted the ruff branch March 6, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomHall2020 TomHall2020 TomHall2020 requested changes

@LiamPattinson LiamPattinson LiamPattinson left review comments

Assignees

@jatkinson1000 jatkinson1000

Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: Move to ruff for linting
3 participants
@jatkinson1000 @TomHall2020 @LiamPattinson