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.

Sign up for GitHub

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

Jump to bottom

MAINT/STY: misc: remove E501 (line length) lint ignore #19491

Merged
merged 3 commits into from Nov 8, 2023
Merged

MAINT/STY: misc: remove E501 (line length) lint ignore #19491

merged 3 commits into from Nov 8, 2023

Conversation

lucascolley
Copy link
Member

Reference issue

Towards gh-19479.

What does this implement/fix?

Follow-up to gh-19489 for misc.

Additional information

@lucascolley
Copy link
Member Author

@mdhaber does this commit structure look good? The first two commits are just gh-19489.

mdhaber
mdhaber reviewed Nov 8, 2023
View reviewed changes
Copy link
Contributor

@mdhaber mdhaber left a comment

Choose a reason for hiding this comment

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

Have you looked for unspecific uses of noqa in misc that are being used just for E501?

scipy/misc/_common.py Outdated
@@ -332,7 +334,7 @@ def electrocardiogram():
>>> plt.ylabel("Power spectrum of the ECG in mV**2")
>>> plt.xlim(f[[0, -1]])
>>> plt.show()
"""
""" # noqa: E501
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because of the URL? By changing the label to something shorter than "_record 208", I think this can fit within the new limit.

Copy link
Contributor

@mdhaber mdhaber Nov 8, 2023 •
edited

Choose a reason for hiding this comment

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

Actually, when I run python tools/lint.py scipy/misc/_common.py locally on main (with E501 exception removed from lint.toml ), I don't see an error here. Any guesses why?

Copy link
Member Author

Choose a reason for hiding this comment

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

The correct usage is python tools/lint.py --files scipy/misc/_common.py, which shows the error for me locally.

Edit: no you're right, I only get the error on line 229. Maybe the linter is happy because it's in a docstring. Will remove the noqa.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like it doesn't apply to docstrings: astral-sh/ruff#1784 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, the code for doc-length is W505

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, E501 actually does apply to docstrings, but it lets URLs run over 👍

scipy/misc/_common.py Outdated Show resolved Hide resolved
@lucascolley
Copy link
Member Author

Have you looked for unspecific uses of noqa in misc that are being used just for E501?

Just checked and there are no unspecific uses of noqa in misc. The majority (51/81) of the unspecific uses are in stats or optimize.

@j-bowhay
Copy link
Member

j-bowhay commented Nov 8, 2023

misc is actually deprecated so I wonder if we just leave it be and focus on the other subpackages?

@lucascolley
Copy link
Member Author

misc is actually deprecated so I wonder if we just leave it be and focus on the other subpackages?

I had considered this but I thought we may as well deal with it for completeness as it's so quick.

@lucascolley lucascolley marked this pull request as ready for review November 8, 2023 10:27
@mdhaber mdhaber merged commit 6ea14ff into scipy:main Nov 8, 2023
19 of 22 checks passed
@mdhaber
Copy link
Contributor

mdhaber commented Nov 8, 2023 •
edited

Looks good! In the next PR, please have CI show that the lint check fails (adding a dummy change to a file if needed), then add a commit that fixes it. Thanks!

@j-bowhay j-bowhay added maintenance Items related to regular maintenance tasks scipy.misc labels Nov 8, 2023
@j-bowhay j-bowhay added this to the 1.12.0 milestone Nov 8, 2023
@j-bowhay
Copy link
Member

j-bowhay commented Nov 8, 2023

@mdhaber that sounds like a lot of CI time since we don't have a skip for everything but lint. I would be happy to review and verify locally by checking out the intermediary commit eg. 290c7da in this case

@mdhaber
Copy link
Contributor

mdhaber commented Nov 8, 2023

that sounds like a lot of CI time since we don't have a skip for everything but lint

This is an argument for having a skip for everything but lint! PRs welcome : )

@lucascolley lucascolley deleted the line-length-misc branch November 8, 2023 21:04
@lucascolley lucascolley mentioned this pull request Nov 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdhaber mdhaber mdhaber left review comments

@j-bowhay j-bowhay j-bowhay approved these changes

@larsoner larsoner Awaiting requested review from larsoner larsoner is a code owner

@rgommers rgommers Awaiting requested review from rgommers rgommers is a code owner

Assignees
No one assigned
Labels
maintenance Items related to regular maintenance tasks scipy.misc
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lucascolley @j-bowhay @mdhaber