MDAnalysis / UserGuide

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

Document darker usage #274

Closed pgbarletta closed 1 year ago

pgbarletta commented 1 year ago

Fixes #247

This is another section of the documentation where this issue is discussed, though it does recommend using black as a formatter, which kind of goes against the philosophy behind the usage of darker, since black doesn't allow partial reformatting.

Let me know if you want me to harmonize the 2 sections.

IAlibay commented 1 year ago

I think there's a misconception here, we never ask folks to pass the darker requirements, only the flake8 ones. The only reason we have the darker bot is because there are no other tools out there that run a minimal pep8 / flake8 check on diffs. (edit: what I mean here is that I do appreciate the optional label, but we need to make it clear that it's only flake/pep8 on diffs that matter)

pgbarletta commented 1 year ago

but we need to make it clear that it's only flake/pep8 on diffs that matter)

right, I thought this line was supposed to show them just that:

darker --diff -r upstream/develop package/MDAnalysis -L flake8

Or does flake8 enforce other checks beyond PEP8?

If so, how do users know the difference between a PEP8 violation like this one:

image

and any other formatting requirement like breaking lists into 1-element-per-line?

IAlibay commented 1 year ago

I'm not on my desktop at the moment, but if I remember correctly, that line will show both black and flake8 issues on the diff? I think what should be emphasized is that the only thing that absolutely needs addressing are the actual flake8 warnings at the bottom of the report, not the full output.

My point being that if they want to black format the whole diff that's fine, but if it conflicts with another linter they enjoy or readability then that's their call.

pgbarletta commented 1 year ago

Ok, now I get it. I think this should do.