search

MDAnalysis / UserGuide

User Guide for MDAnalysis
https://userguide.mdanalysis.org
22 stars 33 forks source link

dev: run pre-commit-hooks #270

Closed jandom closed 1 year ago

jandom commented 1 year ago

Just an experiment to see how many tests fail (I suspect many!) update: turned out not to be true, everything passed!

This is not even running black or isort, just a simple whitespace cleanup. Most of the files changed are automatically modified here, maybe it'll be best to break this up into 2 PRs (one for the actual changes, one for the auto-changes)?

More importantly, even this simple change will make git blame hard to interpret... In general, I'd propose just steam-rolling black and other linters over this codebase because it's unlikely to be 'git blamed' frequently (maybe not?) and would give a good testing ground (how violent is the change, how valuable is the cleanup)

RMeli commented 1 year ago

Sorry @jandom, I missed this PR. Indeed, everything seems to have passed.

Since this PR is just removing whitespaces I think it might be easier to convince people to accept it (compared to black/isort)?

maybe it'll be best to break this up into 2 PRs

However yes, I think breaking into a PR with the actual changes and a PR with formatting is needed, so that the commit with only the formatting can be ignored.

it's unlikely to be 'git blamed' frequently

As mentioned in #269 and https://github.com/MDAnalysis/mdanalysis/issues/2450, git blame is no longer an issue. git blame can now ignore specific commits.

Personally I'd go with black and isort too, but as mentioned in #269 not everyone agrees. I think more and more people are changing their mind about black (especially since https://github.com/MDAnalysis/mdanalysis/issues/2450 was opened), so it might happen eventually but there needs to be consensus. Please add your opinion/vision in https://github.com/MDAnalysis/mdanalysis/issues/2450.

jandom commented 1 year ago

Let's take it a step at a time, and start with removing just whitespaces. Then depending on the appetite, I'd push for the other tools. Thank you for your reply @RMeli !