search

NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
13 stars 3 forks source link

Introduce pre-commit hooks #310

Closed Radonirinaunimi closed 1 month ago

Radonirinaunimi commented 2 months ago

My initial idea was to introduce pre-commit hooks to lint and format the python files, but then I stumbled upon https://github.com/NNPDF/pineappl/issues/180 and decided to introduce some minimal hooks for Rust and all the other files.

The configuration is kept to be very minimal in order to only perform the most necessary steps before pushing commits.

Left to do:

alecandido commented 2 months ago

I like it if someone else is doing the trivial stuff for me 🙃

If it's trivial, it would take no time to you as well :P

I.e. use the word trivial with a large amount of caution

Radonirinaunimi commented 2 months ago

It seems trivial indeed but I had to fight with python files which do not actually contain "python codes" (which in the end is time-consuming) 😇.

Btw, @cschwan, is there a reason why pineappl_cli/src/plot.py can't just be modified into plot.txt or something?

felixhekhorn commented 2 months ago

Btw, @cschwan, is there a reason why pineappl_cli/src/plot.py can't just be modified into plot.txt or something?

for sure we want the Python syntax highlight, because it is almost a Python file (so I'd prefer the .py).

However, I would much prefer making this a true Python script as discussed in https://github.com/NNPDF/pineappl/issues/249 and split the static stuff in a true Python file and then all the dynamic stuff can either be inlined (in Rust) or could then go to a config_plot.py.txt

cschwan commented 2 months ago

I thought about improving the plot script, but it didn't go anywhere. You're welcome to try to improve it yourself. The only conditions for improving it are:

  1. the only dependencies you can rely on are: numpy, matplotlib and the ones already provided by Python.
  2. everything has to be in one self-contained file.
cschwan commented 1 month ago

@Radonirinaunimi please don't fix clippy's warnings, this is out of scope of this PR. Also don't silence them, because at some point we really want to fix them. For the time being we should limit the changes in master to bugfixes and build-system related issues (like this PR) because the main development is going on in v1-file-format, which will change most of the code in the pineappl crate.

Radonirinaunimi commented 1 month ago

@Radonirinaunimi please don't fix clippy's warnings, this is out of scope of this PR. Also don't silence them, because at some point we really want to fix them. For the time being we should limit the changes in master to bugfixes and build-system related issues (like this PR) because the main development is going on in v1-file-format, which will change most of the code in the pineappl crate.

Agreed with limiting the scope of this PR to these. I will now revert the last commit.

But as a side note, fixing all the clippy warnings, as was done in 9fbb246, introduces not that many changes (as opposed to what I initially expected). The issue is that doing so changes the numerical values in a few places, which might not be worrying but for some lead the tests to fail.

cschwan commented 1 month ago

The issue is that doing so changes the numerical values in a few places, which might not be worrying but for some lead the tests to fail.

That's the problem I feared would appear. The evolution code, for instance, is quite fragile when it comes to numerical tolerances and will lead failures in unexpected places.

For the future: keep in mind that we will always want to prefer #[expect(...)] instead of #[allow(...)]. That's a brand new feature, but starting with v1 we'll be able to raise the MSRV. The warnings of the type item in documentation is missing backticks is a false-positive, and they're fixed by adding a clippy.toml (see commit 88f9151b96327c59760abc8c5ec7ff8cdf020be2).

Radonirinaunimi commented 1 month ago

For the future: keep in mind that we will always want to prefer #[expect(...)] instead of #[allow(...)]. That's a brand new feature, but starting with v1 we'll be able to raise the MSRV. The warnings of the type item in documentation is missing backticks is a false-positive, and they're fixed by adding a clippy.toml (see commit 88f9151).

Thanks! That's very useful to know.

Radonirinaunimi commented 1 month ago

Thanks @cschwan for fixing various details and polishing this. I guess this also can be merged?

cschwan commented 1 month ago

Yes, I'll let you have the honours :smiley:!