search

NSLS-II / edrixs

An open source toolkit for simulating RIXS spectra based on ED
https://nsls-ii.github.io/edrixs
GNU General Public License v3.0
34 stars 20 forks source link

Fixed issue #143 -> Add pre-commit #144

Closed Anselmoo closed 2 years ago

Anselmoo commented 2 years ago

Fixed issue #143

  1. Add pre-commit
  2. Apply pre-commit to all files
  3. Add pre-commit to docs
mpmdean commented 2 years ago

Thanks @Anselmoo.

To my untrained eye, I would anticipate this being good practice and not something that will cause any issues. Something that jumps out in the file review is that quite a few Fortran and Fortan configuration files get touched in this change.

Having tried to run some of the Cowan and related codes, I know that Fortran can sometimes be annoyingly sensitive to things like whitespace and new lines. Can @shenmidelin and @mrakitin have a look here? Thanks!

Anselmoo commented 2 years ago

Thanks @Anselmoo.

To my untrained eye, I would anticipate this being good practice and not something that will cause any issues. Something that jumps out in the file review is that quite a few Fortran and Fortan configuration files get touched in this change.

Having tried to run some of the Cowan and related codes, I know that Fortran can sometimes be annoyingly sensitive to things like whitespace and new lines. Can @shenmidelin and @mrakitin have a look here? Thanks!

Thx for the quick feedback @mpmdean

Indeed, we can leave FORTRAN code out! Due to the comment in PR #143, I thought it is a good point to track this in a general way. Especially the python code contains kind of a lot of whitespaces.

What I personally like a lot is the fact, that you can include all linters (black, mypy, pydocstyle, ...) straight into the pre-commit. In this regard, this hook for FORTRAN might be interesting? https://github.com/pseewald/fprettify and https://github.com/pseewald/fprettify/blob/master/.pre-commit-hooks.yaml

mpmdean commented 2 years ago

I'd propose to apply the pre-commit to only .py and .rst. I presume that's easily done? What do other think?

BTW. I believe all our important python passes these. Just ours docs are exempt for now, which is probably lazyness on my part.

Anselmoo commented 2 years ago

That's a good idea to start with python, rst, and maybe JSONfirst and include other formats, later. I will exclude the rest out of the general pre-commit hook.

Anselmoo commented 2 years ago

Looks good to me. I am happy with the hook applied to the .py and .rst files, not sure about the .f90, as @mpmdean mentioned that can break the code.

@Anselmoo, can you please explain how it's supposed to work? Will the workflow automatically commit the linted changes, or will it just fail and a human will need to fix things manually? I am not currently using any pre-commit hooks, would be good to learn about them more. Thanks!

In general https://github.com/pre-commit/pre-commit is python package, which have first install via

  1. pre-commit install
  2. pre-commit run --all-files is good for the first run to check current status quo
  3. Next every git-push will run over pre-commit and block if the commit does not fulfill the requirements. In the case of whitespace, isort or black, it will automatically reformat and you have to re-stash again. In the case of flake8 it will block so long you are not fixing the fail.

If you want, we can add an extended comment in the README. I will restore the *.f90 files and include an exclude.

mrakitin commented 2 years ago

Thanks! I think it'd make sense to include the comment about setting up the recommit hooks in the dev section of the docs and/or README.

Anselmoo commented 2 years ago

Sorry @mpmdean for Late reply. As mentioned in the code review, it is end of line trimming. Sometimes it was previously included, sometimes not. Now, it's consistent However, if you prefer, I will keeps src completely untouched?

mpmdean commented 2 years ago

Sorry @mpmdean for Late reply. As mentioned in the code review, it is end of line trimming. Sometimes it was previously included, sometimes not. Now, it's consistent However, if you prefer, I will keeps src completely untouched?

If you don't mind let's leave src/* docs/user_guide.lyx github_deploy_key_nsls_ii_edrixs.enc

totally untouched. I'm not knowledgeable enough to know this won't cause problems and since these files are barely read there's not much advantage to cleaning them up.

mpmdean commented 2 years ago

@shenmidelin can you check and sign off on these changes?