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

[fmt] core #4874

Merged
merged 11 commits into from
Jan 8, 2025
Merged

[fmt] core #4874

merged 11 commits into from
Jan 8, 2025

Conversation

RMeli
Copy link
Member

@RMeli RMeli commented Dec 29, 2024

@pep8speaks
Copy link

pep8speaks commented Dec 29, 2024

Hello @RMeli! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 3776:80: E501 line too long (80 > 79 characters)

Line 82:80: E501 line too long (84 > 79 characters)
Line 91:80: E501 line too long (87 > 79 characters)
Line 168:80: E501 line too long (88 > 79 characters)
Line 1292:80: E501 line too long (82 > 79 characters)

Line 439:80: E501 line too long (81 > 79 characters)

Line 1016:80: E501 line too long (156 > 79 characters)
Line 1308:80: E501 line too long (82 > 79 characters)
Line 1431:80: E501 line too long (94 > 79 characters)

Comment last updated at 2025-01-07 17:15:32 UTC

@RMeli
Copy link
Member Author

RMeli commented Dec 30, 2024

The test failure here seems real. I'm not very familiar with this part of the code, but I think it is due to the following assumption:

# The first line of the original docstring is not indented, but the
# subsequent lines are. We want to dedent the whole docstring.

From black documentation:

Black cleans up leading and trailing whitespace of docstrings, re-indenting them if needed. It’s been one of the most popular user-reported features for the formatter to fix whitespace issues with docstrings. While the result is technically an AST difference, due to the various possibilities of forming docstrings, all real-world uses of docstrings that we’re aware of sanitize indentation and leading/trailing whitespace anyway.

@RMeli
Copy link
Member Author

RMeli commented Dec 30, 2024

The following change appears to fix things:

-    # The first line of the original docstring is not indented, but the
-    # subsequent lines are. We want to dedent the whole docstring.
-    first_line, other_lines = method.__doc__.split("\n", 1)
-    stub_method.__doc__ = (
-        first_line + "\n" + textwrap.dedent(other_lines) + "\n\n" + annotation
-    )
+    # The original docstring is arrumed to be formatted with black
+    stub_method.__doc__ = textwrap.dedent(method.__doc__) + "\n\n" + annotation

@RMeli RMeli requested a review from jbarnoud December 30, 2024 21:05
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 97.71529% with 13 lines in your changes missing coverage. Please review.

Project coverage is 93.63%. Comparing base (1eca9f2) to head (de6e3d0).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
package/MDAnalysis/core/groups.py 95.75% 5 Missing and 4 partials ⚠️
package/MDAnalysis/core/topologyattrs.py 99.31% 2 Missing ⚠️
package/MDAnalysis/core/_get_readers.py 90.90% 1 Missing ⚠️
package/MDAnalysis/core/topologyobjects.py 96.96% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4874      +/-   ##
===========================================
- Coverage    93.66%   93.63%   -0.03%     
===========================================
  Files          177      189      +12     
  Lines        21796    22861    +1065     
  Branches      3067     3067              
===========================================
+ Hits         20415    21407     +992     
- Misses         929     1002      +73     
  Partials       452      452              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RMeli
Copy link
Member Author

RMeli commented Jan 1, 2025

This one is also good to go IMO. Please check the change described in #4874 (comment).

@RMeli RMeli mentioned this pull request Jan 1, 2025
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

I am updating the comment on the doc-dedent code change (good that you found this interaction — in a way that's been pretty brittle as it required that doc strings were written in a specific way).

Otherwise I did spot checks and believe it's fine.

package/MDAnalysis/core/topologyattrs.py Outdated Show resolved Hide resolved
@RMeli RMeli merged commit 5eef341 into MDAnalysis:develop Jan 8, 2025
24 checks passed
@RMeli RMeli deleted the fmt-core branch January 8, 2025 21:12
Copy link
Contributor

@marinegor marinegor left a comment

Choose a reason for hiding this comment

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

Retrospective LGTM (except for one trailing comma, but it's non-blocking especially given that PR is already merged)

[2, 1, 1],
[3, 1, 1]],
dtype=np.float32)
coords = np.array([[1, 1, 1], [2, 1, 1], [3, 1, 1]], dtype=np.float32)
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps should be this:

coords = np.array([[1, 1, 1], [2, 1, 1], [3, 1, 1],], dtype=np.float32)

(with a trailing comma to maintain 3x3 format)

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 this pull request may close these issues.

4 participants