Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update line length in Contributor guide #347

Closed
orbeckst opened this issue Dec 1, 2023 · 10 comments · Fixed by #348
Closed

update line length in Contributor guide #347

orbeckst opened this issue Dec 1, 2023 · 10 comments · Fixed by #348
Assignees

Comments

@orbeckst
Copy link
Member

orbeckst commented Dec 1, 2023

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.

@orbeckst orbeckst self-assigned this Dec 1, 2023
orbeckst added a commit that referenced this issue Dec 1, 2023
@IAlibay
Copy link
Member

IAlibay commented Dec 1, 2023

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
Copy link
Member Author

orbeckst commented Dec 1, 2023

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

In MDAnalysis/mdanalysis#4292 (comment) 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
Copy link
Member Author

orbeckst commented Dec 1, 2023

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
Copy link
Member

IAlibay commented Dec 1, 2023

:/ 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
Copy link
Member

IAlibay commented Dec 1, 2023

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

@orbeckst
Copy link
Member Author

orbeckst commented Dec 1, 2023

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
Copy link
Member Author

orbeckst commented Dec 1, 2023

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

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

@IAlibay
Copy link
Member

IAlibay commented Dec 1, 2023

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
Copy link
Member

IAlibay commented Dec 1, 2023

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

@orbeckst
Copy link
Member Author

orbeckst commented Dec 1, 2023

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.

lilyminium added a commit that referenced this issue Jan 15, 2024
* updated line length in code contrib section

- fix #347
- add note on black

* trimmed trailing whitespace

* revert back to 79 chars as default

* encouraged use of darker instead of black

* Update doc/source/contributing_code.rst

Co-authored-by: Lily Wang <[email protected]>

---------

Co-authored-by: Lily Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants