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

Remove blanks at line endings #140

Closed
wants to merge 1 commit into from
Closed

Remove blanks at line endings #140

wants to merge 1 commit into from

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Aug 15, 2020

Signed-off-by: Stefan Weil [email protected]

@codecov
Copy link

codecov bot commented Aug 15, 2020

Codecov Report

Merging #140 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #140   +/-   ##
=======================================
  Coverage   37.77%   37.77%           
=======================================
  Files           9        9           
  Lines         998      998           
  Branches      212      212           
=======================================
  Hits          377      377           
  Misses        555      555           
  Partials       66       66           
Impacted Files Coverage Δ
ocrd_tesserocr/binarize.py 22.95% <ø> (ø)
ocrd_tesserocr/crop.py 13.51% <ø> (ø)
ocrd_tesserocr/deskew.py 17.34% <ø> (ø)
ocrd_tesserocr/segment_line.py 63.63% <ø> (ø)
ocrd_tesserocr/segment_region.py 53.64% <ø> (ø)
ocrd_tesserocr/segment_table.py 0.00% <ø> (ø)
ocrd_tesserocr/segment_word.py 76.66% <ø> (ø)
ocrd_tesserocr/recognize.py 47.75% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a370312...96227d7. Read the comment docs.

@kba kba requested a review from bertsky August 16, 2020 11:03
Copy link
Member

@kba kba left a comment

Choose a reason for hiding this comment

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

Trailing newlines are a bit annoying for me too but not so much that I want to mess with the git blame output (yes, there is -w but still).

@stweil
Copy link
Contributor Author

stweil commented Aug 16, 2020

We cannot avoid handling this problem: good editors remove such blanks automatically, so any time I edit one of those files, I am confronted with the problem, and so I am when I run git diff which shows the blanks considered wrong in red. That's why I prefer to clean the code in one step (and maybe add a git rule which prevents future blanks at line endings).

Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

Almost all of these are empty lines. This topic has come up over and over again, and I still haven't heard a good counter-argument:

Blank lines inside comments used to be recommended including proper whitespace indentation in PEP8. But that changed at some point. But even today, this convention is necessary if you want to copy code verbatim into the interpreter, and the compiler still accepts both forms. Pylint can be told not to mock the old convention when configured no-space-check=empty-line (as is the case here).

(The old convention also makes paragraph text processing/folding easier, e.g. in Emacs.)

@stweil
Copy link
Contributor Author

stweil commented Dec 6, 2020

I still think it would be good to follow the convention and avoid blanks at line endings, but close this issue to reduce the list of open pull requests.

@stweil stweil closed this Dec 6, 2020
@stweil stweil deleted the clean branch December 6, 2020 20:03
@bertsky
Copy link
Collaborator

bertsky commented Dec 6, 2020

I still think it would be good to follow the convention and avoid blanks at line endings, but close this issue to reduce the list of open pull requests.

Yes, in the meantime I have seen these arguments, i.e. pylint does not support no-space-check=empty-line anymore anyway. So I agree now we should at least phase out the old convention. (I'll try to adapt my editor accordingly.)

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.

3 participants