Skip to content

Commit

Permalink
add pre-commit to docs and remove double mention of isort and black
Browse files Browse the repository at this point in the history
  • Loading branch information
RoyStegeman committed Mar 28, 2024
1 parent 97e8940 commit feeeabd
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 60 deletions.
27 changes: 17 additions & 10 deletions doc/sphinx/source/contributing/python-tools.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,20 @@ See also [*Reviewing pull requests*](reviews). Note that these can typically be
integrated with your editor of choice.

- The [`pylint`](https://www.pylint.org/) tool allows for the catching of
common problems in Python code. The top level
[`.pylintrc` file](https://github.com/NNPDF/nnpdf/blob/master/.pylintrc)
comes with a useful and not overly noisy configuration.
- The [`black` code formatter](https://github.com/psf/black) runs almost
without configuration and produces typically good results. It is good to run
it by default, to avoid spending time on formatting (or arguing about it).
- The [`isort`](https://pycqa.github.io/isort/) library sorts imports
common problems in Python code. The top level
[`.pylintrc` file](https://github.com/NNPDF/nnpdf/blob/master/.pylintrc)
comes with a useful and not overly noisy configuration.
- New Python code should come formatted with
[`black` tool](https://github.com/psf/black) with [our default
configuration](https://github.com/NNPDF/nnpdf/blob/master/pyproject.toml)
- The [`isort`](https://pycqa.github.io/isort/) library sorts imports
alphabetically, and automatically separated into sections and by type.
- [`pre-commit`](https://pre-commit.com/) is a tool that, can automatically
check for stylistic problems in code such as trailing whitespaces or
forgotten debug statements. Our configuration can be found in
[`.pre-commit-configuration.yaml`](https://github.com/NNPDF/nnpdf/blob/master/.pre-commit-configuration.yaml)
and also ensures that `black` and `isort` are run.


## Debugging

Expand All @@ -80,6 +86,7 @@ places in the code. A few alternatives exists when that is not enough:
arbitrary point in the code. Write
```python
import IPython

IPython.embed()
```
at the location of the code you want to debug. You will then be able to
Expand Down Expand Up @@ -150,10 +157,10 @@ for example a function which tests the `plot_pdfs` provider:
```python
@pytest.mark.mpl_image_compare
def test_plotpdfs():
pdfs = ['NNPDF31_nnlo_as_0118']
pdfs = ["NNPDF31_nnlo_as_0118"]
Q = 10
flavours = ['g']
#plot_pdfs returns a generator with (figure, name_hint)
flavours = ["g"]
# plot_pdfs returns a generator with (figure, name_hint)
return next(API.plot_pdfs(pdfs=pdfs, Q=Q, flavours=flavours))[0]
```

Expand Down
87 changes: 37 additions & 50 deletions doc/sphinx/source/contributing/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ linked in the PR.
* The PR should have **at least one developer assigned to it**, whose task it is to [review](reviews) the
code. The PR cannot be merged into master before the reviewer has approved it.

* Before a PR can be merged into master, the **Travis build for it must pass** (see [here](../ci/index.md)).
* Before a PR can be merged into master, the **Travis build for it must pass** (see [here](../ci/index.md)).
Practically, this means that you should find a green tick next to your PR on the relevant [PR
page](https://github.com/NNPDF/nnpdf/pulls). If you instead find a red cross next to your PR, the
reason for the failure must be investigated and dealt with appropriately.
Expand Down Expand Up @@ -58,78 +58,65 @@ at least two). The expected benefits of the policy are:
- It should improve the overall quality of the code.

- It should provide the author of the change with a reasonably quick feedback
loop to discuss the technical details involved in the changes.
loop to discuss the technical details involved in the changes.

- It should make at least two people (the author and the reviewer) familiar
with the changes. It should also ensure that the changes are easy to read
and maintain in the future, and conform to the structure of the rest of the
project.
with the changes. It should also ensure that the changes are easy to read
and maintain in the future, and conform to the structure of the rest of the
project.

### Guidelines for reviewing

The following approach has been found helpful for reviewers, when reviewing pull
requests:

- Make sure you actually understand what the changes are about. Unclear
details should not pass code review. Ask for clarifications, documentation,
and changes in the code that make it more clear. If you are not in the
position of taking the time, consider asking somebody else to help reviewing
the changes. If the changes are big and difficult to comprehend at once,
consider requesting that the author breaks them down in easier to
understand, self contained, pull requests. Note that it is for the authors
to proactively discuss the proposed changes before they become too difficult
for anyone else to follow, and, failing that, it is fair to ask them to go
through the work of making them intelligible.
details should not pass code review. Ask for clarifications, documentation,
and changes in the code that make it more clear. If you are not in the
position of taking the time, consider asking somebody else to help reviewing
the changes. If the changes are big and difficult to comprehend at once,
consider requesting that the author breaks them down in easier to
understand, self contained, pull requests. Note that it is for the authors
to proactively discuss the proposed changes before they become too difficult
for anyone else to follow, and, failing that, it is fair to ask them to go
through the work of making them intelligible.

- Look at the big picture first. Think about whether the overall idea and
implementation is sound or instead could benefit from going in a different
direction. Ideally before a lot of work has gone into fine tuning details.
implementation is sound or instead could benefit from going in a different
direction. Ideally before a lot of work has gone into fine tuning details.


- Review the code in detail. Try to identify areas where the changes
could be clearly improved in terms of clarity, speed or style. Consider
implementing minor changes yourself, although note that there are
trade-offs: People are more likely to assimilate good patterns if they
implement them a few times, which may be a win long term, even if it takes
longer to ship this particular code change.
could be clearly improved in terms of clarity, speed or style. Consider
implementing minor changes yourself, although note that there are
trade-offs: People are more likely to assimilate good patterns if they
implement them a few times, which may be a win long term, even if it takes
longer to ship this particular code change.

- Ideally changes should come with automatic tests supporting their
correctness.

- Use [automated tools](pytoolsqa) which could catch a few extra
problems. In particular
* Do look at the automated tests that run with the PR.
New code should not break them.
* Use [`pylint`](https://www.pylint.org/) with [our default
configuration](https://github.com/NNPDF/nnpdf/blob/master/.pylintrc) to
catch common problems with Python code.
* New Python code should come formatted with
[`black` tool](https://github.com/psf/black) with [our default
configuration](https://github.com/NNPDF/nnpdf/blob/master/pyproject.toml)
* The imports in Python code should be sorted using the
[`isort`](https://pycqa.github.io/isort/) tool with [our default
configuration](https://github.com/NNPDF/nnpdf/blob/master/pyproject.toml)
* Changes in compiled code should be tested in debug mode, with
the address sanitizer enabled. This is done with the
`-DCMAKE_BUILD_TYPE=Debug -DENABLE_ASAN=ON` options in `cmake`.

Some commits corresponding to major cosmetic changes have been collected in
[`.git-blame-ignore-revs`](
https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view
). It is possible to configure the local git to ignore these commits when
running `git blame`:
```
git config blame.ignoreRevsFile .git-blame-ignore-revs
```
correctness.

- Use [automated tools](pytoolsqa) which could catch a few extra problems.
Also don't forget to look at the automated tests that run with the PR. New
code should not break them.

Some commits corresponding to major cosmetic changes have been collected in
[`.git-blame-ignore-revs`](
https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view
). It is possible to configure the local git to ignore these commits when
running `git blame`:
```
git config blame.ignoreRevsFile .git-blame-ignore-revs
```
- Regardless of automated tests, always run code with the new changes
manually. This gives great insight into possible pitfalls and areas of
improvement.
- Make sure the changes are appropriately documented: Interface functions
should come with rich docstrings, ideally with examples, larger pieces of
functionality should come with some prose explaining what they are for.
should come with rich docstrings, ideally with examples, larger pieces of
functionality should come with some prose explaining what they are for.
- Consider the effects on the larger system: Did this change make some example
or piece of documentation obsolete and therefore mean needs to be updated?
Expand Down

0 comments on commit feeeabd

Please sign in to comment.