Linting RAiDER codebase #672
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes ruff was able to make with `ruff check --fix`
D212 is "no line break at the beginning of a docstring". Conforming to it fully will involve more than what the automatic fix for this category does (it disregards that the description must also be short enough to fit on one line), so I'm choosing to have ruff ignore it for now. Remind me and I'll make a separate issue for this.
Ruff found many more things to change after adding settings to the pyproject.toml. These are the ones it was able to fix automatically.
- Imports sorted
- Quotes """ used in place of apostrophes ''' for docstrings
- Spacing after imports made consistent
- Docstring header consistency (headers are title case with an underline of equals signs and no colon at the end)
- Use of '{}'.format() replaced with f'{}' where possible
- Explicit open() flag 'r' removed as it is the default
- Ensure every docstring description has punctuation - Fixed some typos - Put some comments into imperative mood; will do this for all of them later - Add type annotations where ruff can infer them
8 tasks
- Use double quotes instead of single quotes, except where using double quotes would prevent you from having to escape an apostrophe - Removed unnecessary parentheses - Exactly 2 spaces before a same-line comment - Function calls formatted - Exactly 1 space on either side of an operator (=, +, -, <, etc.) - 1 line between top-of-file docstring and imports - Space before argument type annotation - Space between equals sign in deafult argument - Trailing comma - Exactly 1 empty line at the end of a file - No empty lines at the beginning of a file - Use ".0" instead of "." to denote float literals - One space after a comma - One # for a comment - No extraneous returns
Two versions of update_yaml existed, the other of which was in tools.RAiDER.aria.prepFromGUNW.py. In order to keep the code clean I extracted the function out to utilFcns.py and redirected imports to point to it instead of the other versions. This function was also linted to use pathlib.Path.
Missing docstrings (D100, D101, D102, D103, D107) ignored for now. Also skipped linting on the GUNW methods that are marked unused. As part of the linting, all functions and methods were given type annotations. This ended up reaching into a few other files, most notably including the GUNW workflow's argument parsing.
ANN101 is adding the type annotation "Self" to every method's self argument. This rule is controversial and in my opinion would be of little benefit for this project. D200 and D205 are both about the exact formatting of docstrings. I am electing not to make any fixes for these rules since it would involve revising so documentation. This could be revisited in the future with project experts' input.
- Added type annotations - Missing docstrings (D100, D101, D102, D103, D107) ignored for now
Use pathlib.Path Use pathlib.Path Use pathlib.Path
|
All tests passing. Phew! Just merged with latest version of dev too. I will have to stop short of the whole tools directory though, because my employment period with Dr. Maurer is set to end when the S&T semester begins again. Hopefully this can already improve the project in the ways I mentioned and set it up for further improvement down the road. |
|
@garlic-os looking pretty good! One final request, can you add a line or two to the CONTRIBUTING.md document on how to format using ruff prior to pushing a PR? Thanks! |
General practice highly recommends against the use of eval when at all possible for security reasons: https://www.codiga.io/blog/python-eval/. I found that getattr could do what eval was meant to do here.
Replaced `except Exception` and `except BaseException` with just `except`. All are functionally equivalent, but ruff sadly only picks up on bare `except`, so this change was necessary to allow these occurrences to be identified by ruff in the future. Unfortunately, `except Exception as e` is still not picked up by ruff.
The following packages are now imported the same across all files: - import datetime as dt - import xarray as xr - import multiprocessing as mp
- Use start non-const variables with lowercase letter
Another test was being overshadowed by a test with the same name. Also parametrized/simplified these tests in the process
jlmaurer
reviewed
Aug 8, 2024
There was a problem hiding this comment.
@garlic-os looks very good overall, just had a few comments you can find in the review.
jlmaurer
approved these changes
Aug 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR aims to eliminate all warnings produced by
ruffin thetools/RAiDER/directory. Ideally, when$ ruff check tools/RAiDERis run, it should produce no output.Motivation and Context
Linting is an important part of preserving a project's code quality, making the code appear more consistent and readable, as well as helping to reveal potential mistakes. Clearer code also generally improves our experience as developers on the project and helps mitigate technical debt. With ruff's help I even identified a small bug (#669). As a preliminary step toward instituting a lint check in RAiDER's CI pipeline, the whole project needs to be linted through.
Type of change
Checklist:
I have written new tests for your core changes, as applicable.My change requires a change to the documentation.I have updated the documentation accordingly.