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

Fix W391 linter issues #35109

Merged
merged 1 commit into from
Feb 13, 2023
Merged

Fix W391 linter issues #35109

merged 1 commit into from
Feb 13, 2023

Conversation

tobiasdiez
Copy link
Contributor

πŸ“š Description

Fixes a couple of W391 linter issues that were introduced by merging a couple of trac tickets.

πŸ“ Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

βŒ› Dependencies

@tscrim
Copy link
Collaborator

tscrim commented Feb 13, 2023

I strongly disagree with these changes and the fact this is even a part of the linter. It completely disagrees with the specification (https://www.flake8rules.com/rules/W391.html), which says there should be exactly one blank line at the end of a file. After this change, there are 0 blanklines at the end. They either need to learn how to count or change their specification. I also find it more difficult to read without that blankline.

@tobiasdiez
Copy link
Contributor Author

I didn't introduce this change. It has been enabled in fba8e59. Currently, the linter on the develop branch fails because of this: https://github.com/sagemath/sage/actions/runs/4160169660/jobs/7196912078, which also make it fail for all PRs based on it. So can we please get this in?

@tscrim
Copy link
Collaborator

tscrim commented Feb 13, 2023

I'm sorry, but that isn't really an argument. I could just as well introduce a PR that removes that as a requirement with one person agreeing to it as I don't recall us setting that as a standard. If I missed it, then I will just grumble quietly. However, I have created a topic on sage-devel asking about what "one blankline" means: https://groups.google.com/u/2/g/sage-devel/c/SsghcSi6mfw

@fchapoton
Copy link
Contributor

If there is a controversy, it should rather be debated on sage-devel, no ? In my opinion, it is the implementation that really says the expected meaning of these warnings. I agree that the description given by flake8rules is wrong. The one in
https://pycodestyle.pycqa.org/en/2.9.0/intro.html#error-codes is not precise either.

@tobiasdiez
Copy link
Contributor Author

@tscrim
Copy link
Collaborator

tscrim commented Feb 13, 2023

Okay, I think I understand. They are counting the number of newline characters at the end, which are really "end-of-line" characters. For example, hexdump -C src/sage/algebras/all.py yields

00000890  67 65 62 72 61 73 2e 63  6c 75 73 74 65 72 5f 61  |gebras.cluster_a|
000008a0  6c 67 65 62 72 61 27 2c  20 27 43 6c 75 73 74 65  |lgebra', 'Cluste|
000008b0  72 41 6c 67 65 62 72 61  27 29 0a 0a 6c 61 7a 79  |rAlgebra')..lazy|
000008c0  5f 69 6d 70 6f 72 74 28  27 73 61 67 65 2e 61 6c  |_import('sage.al|
000008d0  67 65 62 72 61 73 2e 79  61 6e 67 69 61 6e 27 2c  |gebras.yangian',|
000008e0  20 27 59 61 6e 67 69 61  6e 27 29 0a              | 'Yangian').|
000008ec

So my misunderstanding comes from what they are specifying as a "blankline" should be newline character.

I will just grumble quietly from here-on-out about git noting that there is no blankline and not being able to scroll that one extra line. Sorry for the trouble.

@github-actions
Copy link

Documentation preview for this PR is ready! πŸŽ‰
Built with commit: 94160a4

@tobiasdiez
Copy link
Contributor Author

No worries! Some IDE's have the ability to add a virtual new line at the end of the file. This might be something for you ;-).

Will merge this PR now to fix the CI pipeline.

@tobiasdiez tobiasdiez merged commit 293dd72 into sagemath:develop Feb 13, 2023
@tobiasdiez tobiasdiez deleted the fix-linter branch February 13, 2023 09:52
@mkoeppe
Copy link
Member

mkoeppe commented Feb 13, 2023

about git noting that there is no blankline

git does not complain about missing blank lines. It would complain about a file ending without a newline character at the end.

@tscrim
Copy link
Collaborator

tscrim commented Feb 14, 2023

about git noting that there is no blankline

git does not complain about missing blank lines. It would complain about a file ending without a newline character at the end.

I remember it used to do so. I checked this now and it doesn't do what it used to do previously, unless it was my editor not putting newline characters at the end (which I highly doubt).

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.

None yet

5 participants