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.

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

Introduce pre-commit hooks #310

Merged
merged 21 commits into from
Oct 24, 2024
Merged

Introduce pre-commit hooks #310

merged 21 commits into from
Oct 24, 2024

Conversation

Radonirinaunimi
Copy link
Member

@Radonirinaunimi Radonirinaunimi commented Sep 18, 2024

My initial idea was to introduce pre-commit hooks to lint and format the python files, but then I stumbled upon #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:

@Radonirinaunimi Radonirinaunimi added the enhancement New feature or request label Sep 18, 2024
@Radonirinaunimi Radonirinaunimi linked an issue Sep 18, 2024 that may be closed by this pull request
Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

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

@alecandido
Copy link
Member

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
Copy link
Member Author

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
Copy link
Contributor

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 #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
Copy link
Contributor

cschwan commented Sep 19, 2024

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.

@Radonirinaunimi Radonirinaunimi linked an issue Sep 25, 2024 that may be closed by this pull request
@cschwan
Copy link
Contributor

cschwan commented Oct 1, 2024

@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
Copy link
Member Author

@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
Copy link
Contributor

cschwan commented Oct 1, 2024

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 88f9151).

@Radonirinaunimi
Copy link
Member Author

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.

.github/workflows/release.yml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@Radonirinaunimi
Copy link
Member Author

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

@cschwan
Copy link
Contributor

cschwan commented Oct 24, 2024

Yes, I'll let you have the honours 😃!

@Radonirinaunimi Radonirinaunimi merged commit ff2be58 into master Oct 24, 2024
9 checks passed
@Radonirinaunimi Radonirinaunimi deleted the add-precommit-hooks branch October 24, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pre-commit?
4 participants