search

MDAnalysis / UserGuide

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

fix: warnings when running 'make html' #269

Closed jandom closed 1 year ago

jandom commented 1 year ago

A very minor PR, fixes two warning messages that appear when running 'make html'

/Users/jandom/workspace/MDAnalysis/UserGuide/doc/source/examples/other/parmed_sim.ipynb:21: WARNING: undefined label: '/examples/other/parmed_sim.ipynb#references'

And the missing reference

/Users/jandom/workspace/MDAnalysis/UserGuide/doc/source/generated/topology/groupmethods.txt:11: WARNING: could not find bibtex key "Gray1984"
review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

jandom commented 1 year ago

Thanks for the review! Sorry random question – not really in scope of this PR – how do you feel about things like black, isort in this community? (And then brining these tools in together via pre-commit so that people have a smoother sailing locally)

IAlibay commented 1 year ago

Because of the size of the existing code in the org, we've agreed that black will not be auto applied (at least for now on the core library). I personally don't really mind isort as long as it doesn't add to existing CI burdens.

lilyminium commented 1 year ago

I've now come around to black, especially for larger communities. I agree with Irfan that I wouldn't want to muddle commit history with black, but if we can apply it to diffs only then it's probably on the whole beneficial!

IAlibay commented 1 year ago

There are already at least two other points of discussion for this. Can I ask that this gets moved to one of them please? In the nicest way possible I'd prefer not have to re-make the same points from those discussions here.

IAlibay commented 1 year ago

Also whilst I'm here - working on diffs is problematic for black because it misses out historical style and linespace violations. See our darker action to see how that ends up happening in the wild.

RMeli commented 1 year ago

@jandom Part of the relevant discussion is here: https://github.com/MDAnalysis/mdanalysis/issues/2450.

@lilyminium argument is what convinced me it was not a very good idea to apply black to the whole code base. However, as you can see from my last comment in the aforementioned thread, that is no longer an issue and personally I don't see any good argument against it anymore (many large projects adopted it and have reformatted the whole code base).

jandom commented 1 year ago

Thank you all for responding, I gotta say it made me giggle as this is probably the PR with most discussion after the merge :P

I've only read https://github.com/MDAnalysis/mdanalysis/issues/2450 casually now but sounds like no stone was left unturned.

Sorry to have kicked an old wasps nest! I probed in the gentlest possible way :P What about trying out some of this tooling for auxiliary repos, like this one?