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

add .pre-commit-config.yaml #2026

Merged
merged 6 commits into from
Mar 28, 2024
Merged

add .pre-commit-config.yaml #2026

merged 6 commits into from
Mar 28, 2024

Conversation

RoyStegeman
Copy link
Member

@RoyStegeman RoyStegeman commented Mar 27, 2024

I did not keep known_first_party. I tested it on model_trainer.py and it put the numpy import inside the n3fit imports while separating from n3fit import model_gen (not sure what's going on there). I also realised that we missed e.g. ekobox.

isort seems to recognise that nnpdf_data, validphys and n3fit are part of this repo so I think that's good enough (let's keep an eye on it though).

I'm not if favour of a CI test

@scarlehoff
Copy link
Member

scarlehoff commented Mar 27, 2024

Why did you remove it from the global .toml, wouldn't it be better to have it there?

If black and isort from pre-commit don't recognize it (they should) you can explictly say --config pyproject.toml or similar.

That way one can use black on isort with your ide and make sure that it is using the same options.

(I guess we want to also check -but not autocommit the fixes- as part of the checks)

@RoyStegeman
Copy link
Member Author

Why did you remove it from the global .toml, wouldn't it be better to have it there?

Because pre-commit cannot use the pyproject.toml file pre-commit/pre-commit#1165

If black and isort from pre-commit don't recognize it (they should) you can explictly say --config pyproject.toml or similar.

I thought there was no such option for isort and didn't really see the benefit. But I guess if you want to run it from the ide that's indeed not possible this way. I was wrong though, and isort does have an option --settings-path that I think do the job so I'll change it again.

(I guess we want to also check -but not autocommit the fixes- as part of the checks)

I'm not so sure, there may be cases in which we don't want to do exactly what black or isort wants. Or maybe not. Either way, let's see, if people adopt pre-commit problems about forgetting to run black should become rare.

@scarlehoff
Copy link
Member

I don't know about pre-commit, but the tools you run inside certainly can (this is used in pineko or pinefarm)

@RoyStegeman
Copy link
Member Author

Not for black and isort

@RoyStegeman
Copy link
Member Author

RoyStegeman commented Mar 27, 2024

Not sure what the problem is, but at least black doesn't seem to recognise the settings from pyproject.toml. I also still need to update the mentions of black and isort in the docs, but for now I've wrestled enough with this.

Copy link
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

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

thanks!
Since you are at it, it might be a good idea to remove the link to the old vp guide from https://github.com/NNPDF/nnpdf/tree/master/doc#currently-documented

(I think we already moved part of the stuff to the standard docs)

@RoyStegeman RoyStegeman merged commit a574fec into master Mar 28, 2024
6 checks passed
@RoyStegeman RoyStegeman deleted the add_pre_commit branch March 28, 2024 11:22
@RoyStegeman RoyStegeman added the devtools Build, automation and workflow label Mar 28, 2024
@scarlehoff scarlehoff mentioned this pull request May 20, 2024
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devtools Build, automation and workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants