Formatting experiment

[obackhouse]

Formats the source code using black. Mostly just wanted to gauge opinions on this. Is anyone actually against it, and if so why? I think it makes things much tidier?

[obackhouse]

Based off the wrong branch but it's just for discussion so w/e

[codecov-commenter]

Codecov Report

Patch coverage: 74.28% and project coverage change: -0.22% ⚠️

Comparison is base (3cf3f1f) 71.85% compared to head (2bfab76) 71.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
- Coverage   71.85%   71.64%   -0.22%     
==========================================
  Files         152      152              
  Lines       20066    20173     +107     
  Branches     3263     3334      +71     
==========================================
+ Hits        14418    14452      +34     
- Misses       4852     4891      +39     
- Partials      796      830      +34     

Files Changed	Coverage Δ
vayesta/core/bath/__init__.py	71.05% <ø> (ø)
vayesta/core/bath/ewdmet.py	26.53% <0.00%> (ø)
vayesta/core/fragmentation/__init__.py	100.00% <ø> (ø)
vayesta/core/helper.py	12.69% <0.00%> (ø)
vayesta/core/qemb/register.py	94.44% <ø> (ø)
vayesta/core/scmf/__init__.py	100.00% <ø> (ø)
vayesta/core/symmetry/symmetry.py	0.00% <0.00%> (ø)
vayesta/core/symmetry/tsymmetry.py	0.00% <0.00%> (ø)
vayesta/core/types/wf/__init__.py	100.00% <ø> (ø)
vayesta/core/types/wf/ccsdtq.py	82.14% <ø> (ø)
... and 128 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cjcscott]

I'm pro this- I don't have gigantically strong opinions on any specific formatting rules, and generally think having a consistent coding style throughout the code is very valuable for ease of reading.

[abhishekkhedkar09]

I am definitely pro this. Logical errors are much easier to spot with consistently well-formatted code.

[cjcscott]

@obackhouse Would I be OK to resurrect this branch a bit? I'd be happy with the parameters you've chosen, so would just rebase those on master and rerun?

[obackhouse]

Yep - just force merge and then run black

[cjcscott]

OK, I've updated the black configuration slightly to avoid some errors I was getting. There are two other things I think we should do before merging

• Set up git blame to ignore major reformatting commits
• Consider whether we want to add a pre-commit hook to run black on any changes, avoid the need for manual application. I'd be pro this, but don't have any experience with whether there might be any downsides (if you don't have black installed and don't have any way to download it, would this prohibit development? If so, should we add it as a dependency?)

[obackhouse]

😄

[obackhouse]

OK, I've updated the black configuration slightly to avoid some errors I was getting. There are two other things I think we should do before merging

• Set up git blame to ignore major reformatting commits
• Consider whether we want to add a pre-commit hook to run black on any changes, avoid the need for manual application. I'd be pro this, but don't have any experience with whether there might be any downsides (if you don't have black installed and don't have any way to download it, would this prohibit development? If so, should we add it as a dependency?)

Good idea with the git blame. I'm usually against pre-commit hooks, but I see the pros too. The dependency is fine because there are also development dependencies like setuptools and the documentation dependencies, these are not installed when you install the package normally.

[cjcscott]

OK, I've added a test configuration for a pre-commit into this branch; to set it up you just need to install pre-commit, then run pre-commit install in the vayesta root directory. It has a reasonable initial setup (which I assume requires internet) to set up an environment for black on first use, then is pretty much instantaneous. When making a commit, it'll automatically run black on your changes; if anything is modified it'll stop with those changes present but not staged for commit. This makes it very easy to check what changes have been made; if no changes are made the commit will go through, so all that'll usually be required is adding the black-performed modifications and commiting again.

If you haven't used it before (as I hadn't) I would recommend trying it out to see how easy it is to fit into existing workflows!

I should also say that I've extended this to include formatting on everything, not just the source code. This was initially because getting the pre-commit to respect those limitations was non-trivial, but then also because I realised enforcing a consistent code style everywhere except for the public-facing examples was a bit ridiculous. Obviously revertible if anyone disagrees!

[cjcscott]

(apologies for the force-pushing; just ensuring we only have a single mega-formatting commit)

[obackhouse]

Can the two configuration files in the root directory go in .git, or do they have to be in root? If they have to be there, can you check if they have pyproject.toml support? We should aim to have as few config files in the root directory as possible

[cjcscott]

The file of revisions to ignore has to be in the root directory to be detected by github afaik, which we do probably want; that's going off description here though: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view If we don't care about GitHub detection it has to be set as a personal config anyways, so could be anywhere. As for the pre-commit, I can't find anything and from this it seem like the developers weren't enthusiastic about pyproject.toml (possibly with good reason, I don't know): pre-commit/pre-commit#1165