MDAnalysis / UserGuide

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

update line length in Contributor guide #347

Closed orbeckst closed 10 months ago

orbeckst commented 11 months ago

The Contributor Guide in https://userguide.mdanalysis.org/stable/contributing_code.html#code-formatting-in-python states 79 char line limit but we actually use 88 chars (see .flake8 https://github.com/MDAnalysis/mdanalysis/blob/f4005db383f3f4c446433fb32156236f29f251ba/.flake8#L2 )

The contributor guide should be updated to say 88 chars per line.

IAlibay commented 11 months ago

Is the intent here not to try to enforce 79 but be lenient if it goes to 88? i.e. "do your best but you won't lose marks if you go above the character limit a bit"

orbeckst commented 11 months ago

Our flake8 file says 88; maybe we discussed at some point that 79 was a bit to restrictive?

In https://github.com/MDAnalysis/mdanalysis/pull/4292#discussion_r1411841564 it was noted that black formatted the new file with 88 chars.

At the end of the day, this is not a hill I am going to die on: I just want everything to be consistent.

orbeckst commented 11 months ago

The change to 88 came with the introduction of the darker linter, see blame https://github.com/MDAnalysis/mdanalysis/blame/f4005db383f3f4c446433fb32156236f29f251ba/.flake8#L2 in MDAnalysis/mdanalysis#3954.

(EDIT: added PR)

IAlibay commented 11 months ago

:/ if I remember the conversation properly the intent was to get folks to stick to 79 but to use 88 on CI to reduce burden on reviewers. I personally think it's easier to just "encourage 79 but if you have a reason not to adhere then it's good" (i.e. for comprehension etc..) at the end of the day we really only enforce it when it's really necessary.

IAlibay commented 11 months ago

I should add, we now also have the revived pep8speaks, so our behaviour of darker linting vs pep8speaks isn't consistent.

orbeckst commented 11 months ago

I started suggesting to run black on new files, just so that we don't have to deal with any clean up and if black uses 88 chars then we will get 88 chars consistently. In this case the 79 in the Contribution Guide look like an inconsistency. I can change the language and say that 79 are encouraged but that we allow up to 88.

I assume (but haven't tested myself) that the User Guide recommendation for using darker will also break on 88 chars.

orbeckst commented 11 months ago

Should we then change .flake8 to 79 for consistency??

I reverted to 79 chars for right now in 9af161c6ef0d1d95bbe819645158411d047c48d6.

IAlibay commented 11 months ago

Honestly I'm of the opinion of just killing darker lint 😅 - half the coredevs didn't like it because it was too restrictive and the other half wanted to just use black. At least if we don't run flake8 on CI we won't have to fix it when it dies 🙃

IAlibay commented 11 months ago

Personally, for local linting, I just use the default flake8 settings and it just does its thing.

orbeckst commented 11 months ago

That's all fair ;-). If pep8speaks works again then we could switch off darker.

Feel free to just put a blocking review on my PR.