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

Format graphic_matroid.py #37141

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Conversation

gmou3
Copy link
Contributor

@gmou3 gmou3 commented Jan 22, 2024

Improve the formatting of graphic_matroid.py.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working to clean up the file. However, most of your changes actually make the file harder to understand the code and output. The 80 char/line can often be bad to follow rigidly, which can be seen by line splits with small output for just a few characters over. While we generally follow this more strictly for docstrings, there are good reasons to violate this at times for readability, especially when the html output doesn't require a horizontal scrollbar. So unfortunately I think most of these changes should be reverted.

Also, IMO, the strong avoidance of l because it could be confused with I thing only is an issue if both appear or there is ambiguity with what symbol it should mean. As such, calling labels ll instead of l doesn't actually fix the problem (why isn't it II instead?) but just makes the names longer and more confusing.

@gmou3
Copy link
Contributor Author

gmou3 commented Jan 23, 2024

I agree actually, I mostly wanted to remove the warnings that appeared on my editor. I think it is better now.

@tscrim
Copy link
Collaborator

tscrim commented Jan 25, 2024

I agree actually, I mostly wanted to remove the warnings that appeared on my editor. I think it is better now.

Please change your settings so those warning are disabled or find a new editor please (and launch the current one into the sun :) ). Most of these changes really are making the code harder to read. PEP8 allows this flexibility (in particular, one of the main points is make the code consistent).

@gmou3 gmou3 force-pushed the lint_graphic_matroid branch from 52afd89 to 968759e Compare January 25, 2024 18:39
Revert some changes and enforce line limits: docstrings to 80 and example blocks to 95.
@gmou3 gmou3 force-pushed the lint_graphic_matroid branch from 968759e to d0a1dae Compare January 25, 2024 19:04
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. One last little PEP8 thing since you are modifying the line.

Suggested by tscrim

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Copy link

Documentation preview for this PR (built with commit 82bab29; changes) is ready! 🎉

@tscrim
Copy link
Collaborator

tscrim commented Jan 30, 2024

Thank you.

@vbraun vbraun merged commit 0209c4d into sagemath:develop Feb 2, 2024
16 of 17 checks passed
@gmou3 gmou3 deleted the lint_graphic_matroid branch February 2, 2024 21:47
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants