-
Notifications
You must be signed in to change notification settings - Fork 9
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
Major Refactor: Add Ruff Linting, Nox, and Pre-Commit Hook #266
base: main
Are you sure you want to change the base?
Changes from all commits
58d7c2b
720dcd4
cdda670
2b94867
d25dea2
fbb55c9
e5adecc
870d2d6
dc40707
c802033
5f61b94
b0a392c
48a571d
4302b55
8d44d45
83a3e41
1badbad
3fe5fec
0c50992
7f8aec4
3e78c47
089497c
1ae3d11
1e9fc9c
1d8a57f
e0678b8
8272ba2
34524f5
593fd23
9b5fb54
38a4150
c1fd8bc
4bb4333
7786cad
79e93d3
3bb9ce1
633d489
47acd99
618ef87
43937bc
01bbaf9
bb714bf
8724a2b
fa29088
13f3db2
d3e825c
4aa2765
180a17e
5c16f7e
cec2844
1230ef6
d367c22
5b76b6a
f3eb287
46bc834
e10f8dd
d375f32
07fd3ef
bda1de7
45a9890
d7fbd6e
e623302
4d29fd9
4abbc02
309e040
302e97e
3fba75b
73db7cf
0c720a6
d8e1f6a
7d77488
8555474
bd9b0ec
d1ab319
d028c67
eabaf30
1952d40
5f54512
daa38ee
1203e89
4e0fa0d
f08c142
a5fb657
4150226
b2c5a18
718440d
47aa63d
07ff382
12daada
bd34ed0
bfda72c
3db6e8b
6413588
bb9c581
b1e771b
cca5220
aec45b7
f47db30
1d34294
380112f
eb56114
e6f10f2
4e62eae
1bc6ed1
b2c81cd
a4e2f57
244948d
9d572e7
3c1b3db
9975ef0
a159b1d
5c9e28d
6050344
fb2f5a0
3951c79
c2912b0
e13a7ad
54ebe85
a777133
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
repos: | ||
- repo: https://github.com/astral-sh/ruff-pre-commit | ||
rev: v0.6.8 | ||
hooks: | ||
# Run the formatter. | ||
- id: ruff-format | ||
args: [--config=pyproject.toml] | ||
# Run the linter. | ||
- id: ruff | ||
args: [--config=pyproject.toml] | ||
|
||
|
||
- repo: https://github.com/pre-commit/pre-commit-hooks | ||
rev: v2.3.0 | ||
# note: pre-commit runs top-to-bottom, so put the hooks that modify content first, | ||
# followed by checks that might be more likely to pass after the modifactaion hooks (like flake8) | ||
hooks: | ||
# Checks for large files added to the repository, typically to prevent accidental inclusion of large binaries or datasets. | ||
- id: check-added-large-files | ||
# Detects potential filename conflicts due to case-insensitive filesystems (e.g., Windows) where File.txt and file.txt would be considered the same. | ||
- id: check-case-conflict | ||
# Checks for files that contain merge conflict strings (e.g., <<<<<<<, =======, >>>>>>>). | ||
- id: check-merge-conflict | ||
# Validates YAML files for syntax errors. | ||
- id: check-yaml | ||
# Detects debug statments (e.g., print, console.log, etc.) left in code. | ||
- id: debug-statements | ||
# Ensures files have a newline at the end. | ||
- id: end-of-file-fixer | ||
# Removes trailing whitespace characters from files. | ||
- id: trailing-whitespace |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -100,7 +100,7 @@ git pull upstream main | |||||||||||||||
# update your fork's main branch with any changes from upstream | ||||||||||||||||
git push origin main | ||||||||||||||||
# create and switch to the branch | ||||||||||||||||
git checkout -b my_cool_branch | ||||||||||||||||
git checkout -b my_cool_branch | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
Then, create new changes on this branch and, when you're ready, add and commit them: | ||||||||||||||||
|
@@ -109,24 +109,71 @@ Then, create new changes on this branch and, when you're ready, add and commit t | |||||||||||||||
# stage the changes | ||||||||||||||||
git add src/plenoptic/the_file_you_changed.py | ||||||||||||||||
# commit your changes | ||||||||||||||||
git commit -m "A helpful message explaining my changes" | ||||||||||||||||
git commit -m "A helpful message explaining my changes" | ||||||||||||||||
# push to the origin remote | ||||||||||||||||
git push origin my_cool_branch | ||||||||||||||||
git push origin my_cool_branch | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
If you aren't comfortable with `git add`, `git commit`, `git push`, I recommend the [Software Carpentry git lesson](https://swcarpentry.github.io/git-novice/). | ||||||||||||||||
|
||||||||||||||||
#### Contributing your change back to plenoptic | ||||||||||||||||
|
||||||||||||||||
You can make any number of changes on your branch. Once you're happy with your changes, [add tests](#adding-tests) to check that they run correctly and [add documentation](#adding-documentation), then make sure the existing [tests](#testing) all run successfully, that your branch is up-to-date with main, and then open a pull request by clicking on the big `Compare & pull request` button that appears at the top of your fork after pushing to your branch (see [here](https://intersect-training.org/collaborative-git/03-pr/index.html) for a tutorial). | ||||||||||||||||
You can make any number of changes on your branch. Once you're happy with your changes, [add tests](#adding-tests) to check that they run correctly and [add documentation](#adding-documentation), then make sure the existing [tests](#testing) all run successfully, that your branch is up-to-date with main, and then open a pull request by clicking on the big `Compare & pull request` button that appears at the top of your fork after pushing to your branch (see [here](https://intersect-training.org/collaborative-git/03-pr/index.html) for a tutorial). | ||||||||||||||||
|
||||||||||||||||
Your pull request should include information on what you changed and why, referencing any relevant issues or discussions, and highlighting any portion of your changes where you have lingering questions (e.g., "was this the right way to implement this?") or want reviewers to pay special attention. You can look at previous closed pull requests to see what this looks like. | ||||||||||||||||
|
||||||||||||||||
At this point, we will be notified of the pull request and will read it over. We will try to give an initial response quickly, and then do a longer in-depth review, at which point you will probably need to respond to our comments, making changes as appropriate. We'll then respond again, and proceed in an iterative fashion until everyone is happy with the proposed changes. This process can take a while! (The more focused your pull request, the less time it will take.) | ||||||||||||||||
|
||||||||||||||||
If your changes are integrated, you will be added as a Github contributor and as one of the authors of the package. Thank you for being part of `plenoptic`! | ||||||||||||||||
|
||||||||||||||||
### Style guide | ||||||||||||||||
### Code Quality and Linting | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
We use [Ruff](https://docs.astral.sh/ruff/) for linting and formatting our Python code to maintain a consistent code style and catch potential errors early. To ensure your contributions meet these standards, please follow the guidelines below: | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
#### Using Ruff | ||||||||||||||||
|
||||||||||||||||
Ruff is a fast and comprehensive Python formatter and linter that checks for common style and code quality issues. It combines multiple tools, like Pyflakes, pycodestyle, isort, and other linting rules into one efficient tool, which are specified in `pyproject.toml`. Before submitting your code, make sure to run Ruff to catch any issues. | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
**Using Ruff for [Formatting](https://docs.astral.sh/ruff/formatter/#philosophy):** | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
`ruff format` is the primary entrypoint to the formatter. It accepts a list of files or directories, and formats all discovered Python files: | ||||||||||||||||
```bash | ||||||||||||||||
ruff format # Format all files in the current directory. | ||||||||||||||||
ruff format path/to/code/ # Format all files in `path/to/code` (and any subdirectories). | ||||||||||||||||
ruff format path/to/file.py # Format a single file. | ||||||||||||||||
``` | ||||||||||||||||
For the full list of supported options, run `ruff format --help`. | ||||||||||||||||
|
||||||||||||||||
**Using Ruff for [Linting](https://docs.astral.sh/ruff/linter/):** | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
To run Ruff on your code: | ||||||||||||||||
```bash | ||||||||||||||||
ruff check . | ||||||||||||||||
``` | ||||||||||||||||
It'll then tell you which lines are violating linting rules and may suggest that some errors are automatically fixable. | ||||||||||||||||
|
||||||||||||||||
To automatically fix lintint errors, run: | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
ruff --fix . | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
Be careful with **unsafe fixes**, safe fixes are symbolized with the tools emoji and are listed [here](https://docs.astral.sh/ruff/rules/)! | ||||||||||||||||
|
||||||||||||||||
#### Ignoring Ruff Linting | ||||||||||||||||
You may want to suppress lint errors, for example when too long lines (code `E501`) are desired because otherwise the url might not be readable anymore. | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'm thinking it might be helpful to maintain a list of what we consider acceptable uses of ignore here. Currently you've documented them in this PR, but should they live in this file or on some dedicated issue? or on the discussions board? |
||||||||||||||||
You can do this by adding the following to the end of the line: | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
# noqa: E501 | ||||||||||||||||
``` | ||||||||||||||||
If you want to suppress an error across an entire file, do this: | ||||||||||||||||
```bash | ||||||||||||||||
# ruff: noqa: E501 | ||||||||||||||||
``` | ||||||||||||||||
Comment on lines
+166
to
+172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you put some example python code in both of these cases? for example, do these go above the relevant line, on the same line, etc. |
||||||||||||||||
|
||||||||||||||||
For more details, refer to the [documentation](https://docs.astral.sh/ruff/linter/#error-suppression). | ||||||||||||||||
|
||||||||||||||||
#### General Style Guide Recommendations: | ||||||||||||||||
|
||||||||||||||||
- Longer, descriptive names are preferred (e.g., `x` is not an appropriate name | ||||||||||||||||
for a variable), especially for anything user-facing, such as methods, | ||||||||||||||||
|
@@ -135,6 +182,17 @@ If your changes are integrated, you will be added as a Github contributor and as | |||||||||||||||
(see [below](#docstrings) for details). Hidden ones do not *need* to have | ||||||||||||||||
complete docstrings, but they probably should. | ||||||||||||||||
|
||||||||||||||||
#### Pre-Commit Hooks: Identifying simple issues before submission to code review (and how to ignore those) | ||||||||||||||||
Pre-commit hooks are useful for the developer to check if all the linting and formatting rules (see Ruff above) are honored _before_ committing. That is, when you commit, pre-commit hooks are run and auto-fixed where applicable (e.g., trailing whitespace). You then need to add _again_ if you want these changes to be included in your commit. | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
Should you want to ignore pre-commit hooks, you can add `--no-verify` to your commit message like this: | ||||||||||||||||
```bash | ||||||||||||||||
git commit -m <my commit message> --no-verify | ||||||||||||||||
``` | ||||||||||||||||
Comment on lines
+188
to
+191
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the use case here? pre-commit is optional anyway, but if people have installed it, why would they ignore it? |
||||||||||||||||
|
||||||||||||||||
All of the above only applies, if you have the pre-commit package manager installed using | ||||||||||||||||
`pip install pre-commit`. | ||||||||||||||||
Comment on lines
+193
to
+194
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this line, can we have the setup instructions (and link) for pre-commit? The user will need to install pre-commit in the relevant environment and then run
Suggested change
See pre-commit docs for more details.
|
||||||||||||||||
|
||||||||||||||||
### Adding models or synthesis methods | ||||||||||||||||
|
||||||||||||||||
In addition to the above, see the documentation for a description of | ||||||||||||||||
|
@@ -199,7 +257,124 @@ several choices for how to run a subset of the tests: | |||||||||||||||
View the [pytest documentation](https://doc.pytest.org/en/latest/usage.html) for | ||||||||||||||||
more info. | ||||||||||||||||
|
||||||||||||||||
### Adding tests | ||||||||||||||||
### Using nox to simplify testing and linting | ||||||||||||||||
This section is optional but if you want to easily run tests in an isolated environment | ||||||||||||||||
using the [nox](https://nox.thea.codes/en/stable/) command-line tool. | ||||||||||||||||
|
||||||||||||||||
`nox` is installed automatically as a `[dev]` dependency of plenoptic. | ||||||||||||||||
|
||||||||||||||||
To run all tests and linters through `nox`, from the root folder of the | ||||||||||||||||
plenoptic package, execute the following command, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
nox | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
`nox` will read the configuration from the `noxfile.py` script. | ||||||||||||||||
|
||||||||||||||||
If you want to run just the tests, add the following option, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
nox -s tests | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
for running only the linters, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
nox -s linters | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
and for testing only the coverage, run: | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
nox -s coverage | ||||||||||||||||
``` | ||||||||||||||||
Comment on lines
+277
to
+291
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of this, I think we should tell them to run |
||||||||||||||||
|
||||||||||||||||
`nox` offers a variety of configuration options, you can learn more about it from their | ||||||||||||||||
[documentation](https://nox.thea.codes/en/stable/config.html). | ||||||||||||||||
|
||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
#### Multi-python version testing with pyenv | ||||||||||||||||
Sometimes, before opening a pull-request that will trigger the `.github/workflow/ci.yml` continuous | ||||||||||||||||
integration workflow, you may want to test your changes over all the supported python versions locally. | ||||||||||||||||
|
||||||||||||||||
Handling multiple installed python versions on the same machine can be challenging and confusing. | ||||||||||||||||
[`pyenv`](https://github.com/pyenv/pyenv) is a great tool that really comes to the rescue. | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
This tool doesn't come with the package dependencies and has to be installed separately. Installation instructions | ||||||||||||||||
are system specific but the package readme is very details, see | ||||||||||||||||
[here](https://github.com/pyenv/pyenv?tab=readme-ov-file#installation). | ||||||||||||||||
|
||||||||||||||||
Follow carefully the instructions to configure pyenv after installation. | ||||||||||||||||
|
||||||||||||||||
Once you have tha package installed and configured, you can install multiple python version through it. | ||||||||||||||||
First get a list of the available versions with the command, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
pyenv install -l | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
Install the python version you need. For this example, let's assume we want `python 3.10.11` and `python 3.11.8`, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
pyenv install 3.10.11 | ||||||||||||||||
pyenv install 3.11.8 | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
You can check which python version is currently set as default, by typing, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
pyenv which python | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
And you can list all available versions of python with, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
pyenv versions | ||||||||||||||||
``` | ||||||||||||||||
If you want to run `nox` on multiple python versions, all you need to do is: | ||||||||||||||||
|
||||||||||||||||
1. Set your desired versions as `global`. | ||||||||||||||||
```bash | ||||||||||||||||
pyenv global 3.11.8 3.10.11 | ||||||||||||||||
``` | ||||||||||||||||
This will make both version available, and the default python will be set to the first one listed | ||||||||||||||||
(`3.11.8` in this case). | ||||||||||||||||
2. Run nox specifying the python version as an option. | ||||||||||||||||
```bash | ||||||||||||||||
nox -p 3.10 | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
Note that `noxfile.py` lists the available option as keyword arguments in a session specific manner. | ||||||||||||||||
|
||||||||||||||||
If you have multiple python version installed, we recommend to manage your virtual environments | ||||||||||||||||
through `pyenv`. For that you'll need to install the extension | ||||||||||||||||
[`pyenv-virtualenv`](https://github.com/pyenv/pyenv-virtualenv). | ||||||||||||||||
Comment on lines
+349
to
+351
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
This tool works with most of the environment managers including (`venv` and `conda`). | ||||||||||||||||
Creating an environment with it is as simple as calling, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
pyenv virtualenv my-python my-enviroment | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
Here, `my-python` is the python version, one between `pyenv versions`, and `my-environment` is your | ||||||||||||||||
new environment name. | ||||||||||||||||
|
||||||||||||||||
If `my-python` has `conda` installed, it will create a conda environment, if not, it will use `venv`. | ||||||||||||||||
|
||||||||||||||||
You can list the virtual environment only with, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
pyenv virtualenvs | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
And you can uninstall an environment with, | ||||||||||||||||
|
||||||||||||||||
```bash | ||||||||||||||||
pyenv uninstall my-environment | ||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
### Adding tests | ||||||||||||||||
|
||||||||||||||||
New tests can be added in any of the existing `tests/test_*.py` scripts. Tests | ||||||||||||||||
should be functions, contained within classes. The class contains a bunch of | ||||||||||||||||
|
@@ -261,7 +436,7 @@ metamer instances run for. We do this using | |||||||||||||||
`vgg16_synth_max_iter 10`; note that you need a `-p` for each parameter and | ||||||||||||||||
you should change nothing else about that line). See the block with `if: ${{ | ||||||||||||||||
matrix.notebook == 'examples/Demo_Eigendistortion.ipynb' }}` for an example. | ||||||||||||||||
|
||||||||||||||||
A similar procedure could be used to reduce the size of an image or other steps | ||||||||||||||||
that could similarly reduce the total time necessary to run a notebook. | ||||||||||||||||
|
||||||||||||||||
|
@@ -395,7 +570,7 @@ header (you can have as many sub-headers as you'd like). | |||||||||||||||
|
||||||||||||||||
You should [build the docs yourself](#build-the-documentation) to ensure it | ||||||||||||||||
looks correct before pushing. | ||||||||||||||||
|
||||||||||||||||
#### Images and plots | ||||||||||||||||
|
||||||||||||||||
You can include images in `.rst` files in the documentation as well. Simply | ||||||||||||||||
|
@@ -405,9 +580,9 @@ place them in the `docs/images/` folder and use the `figure` role, e.g.,: | |||||||||||||||
.. figure:: images/path_to_my_image.svg | ||||||||||||||||
:figwidth: 100% | ||||||||||||||||
:alt: Alt-text describing my image. | ||||||||||||||||
|
||||||||||||||||
Caption describing my image. | ||||||||||||||||
|
||||||||||||||||
``` | ||||||||||||||||
|
||||||||||||||||
To refer to it directly, you may want to use the [numref | ||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? did this come from one of the linters or elsewhere?