PR: Check there is one empty line at the end of the file #371
Conversation
There was a problem hiding this comment.
Looks pretty good. Thanks for writing this @goanpeca . 😄
Thinking if we can simplify the check a little bit more. Also giving some thought to Python 2/3 compatibility and unicode. If you have some thoughts along these lines, please feel free to share.
| # 11: There should be one empty line at the end of the file. | ||
| if recipe_dir is not None and os.path.exists(meta_fname): | ||
| with open(meta_fname, 'r') as f: | ||
| lines = f.read().split('\n') |
There was a problem hiding this comment.
I think splitlines behaves a little better when it comes to Python 2/3 compatibility and unicode. However, I don't know that we will get the behavior we want then. Not sure what we should do about it.
Maybe we need start using unicode literals with the linter. That has fixed some problems in other areas. Also I expect this will fix some other linter issues with unicode that we have experienced.
There was a problem hiding this comment.
Yeah, I thought of this, will use splitlines 👍
There was a problem hiding this comment.
So the trick I found at least is splitlines with one terminal newline and zero ends up being the same. 😞 Not sure if it is better in that case.
We can keep the the end line characters though. I'm just not sure if that makes it better or not.
| lines = f.read().split('\n') | ||
|
|
||
| end_empty_lines_count = 0 | ||
| for i, line in enumerate(reversed(lines)): |
There was a problem hiding this comment.
Could use start=1 with enumerate to simplify this a bit.
| end_empty_lines_count = 0 | ||
| for i, line in enumerate(reversed(lines)): | ||
| if line != '': | ||
| break |
There was a problem hiding this comment.
Wondering if we can reduce this some application of a function from itertools.
There was a problem hiding this comment.
Maybe, this is a naive but working execution, I can optimize
There was a problem hiding this comment.
You mean something like sum(1 for _ in itertools.takewhile(lambda x: x == '', reversed(lines))) but if that's really better? 😄
There was a problem hiding this comment.
Hehe, yeah, maybe there is a super pythonic and readable way of making that a one liner, but yeah, I dont know if it is really better. Thanks for the example @MSeifert04 :-)
There was a problem hiding this comment.
Maybe two lines:
trailing_empty_lines = itertools.takewhile(lambda x: x == '', reversed(lines))
num_trailing_empty_lines = sum(1 for _ in trailing_empty_lines) # or len(list(trailing_empty_lines))
| for i, line in enumerate(reversed(lines)): | ||
| if line != '': | ||
| break | ||
| end_empty_lines_count = i + 1 |
There was a problem hiding this comment.
With start=1 this is then just end_empty_lines_count = i.
| break | ||
| end_empty_lines_count = i + 1 | ||
|
|
||
| if lines[-1] != '' or end_empty_lines_count != 1: |
There was a problem hiding this comment.
Why do we need both checks? Are they not always the same?
There was a problem hiding this comment.
No, check 1 takes into account that there is no empty line at the end
Check 2 takes into account more than 1 empty line at the end
There was a problem hiding this comment.
Ah yes you are right check 2 is enough :-p
There was a problem hiding this comment.
No worries. Was wondering again if I was missing something. 😄
| if content == valid_content: | ||
| self.assertNotIn(expected_message, lints) | ||
| else: | ||
| self.assertIn(expected_message, lints) |
| @@ -122,6 +122,20 @@ def lintify(meta, recipe_dir=None): | |||
| lints.append('The recipe `license` should not include the word ' | |||
| '"License".') | |||
|
|
|||
| # 11: There should be one empty line at the end of the file. | |||
| if recipe_dir is not None and os.path.exists(meta_fname): | |||
| with open(meta_fname, 'r') as f: | |||
| b'extra:\n recipe-maintainers:\n - goanpeca\n\n\n', | ||
| b'extra:\r recipe-maintainers:\r - goanpeca\r\r\r', | ||
| b'extra:\r\n recipe-maintainers:\r\n - goanpeca\r\n\r\n\r\n', | ||
| ] |
There was a problem hiding this comment.
Do you feel safer now @jakirkham ?
Not sure what you meant by unicode py2/3 problems, python always transforms the correct ending into '\n' (as the test shows)
There was a problem hiding this comment.
Sorry, was trying to save you from going down this road by trying to handle unicode cleanly throughout the linter and associated tests. ( #372 )
I think it should be ok to make these explicitly unicode. Though I'll let you be the judge.
The \r* tests open an interesting question of should we permit this. 😄 Though I see no harm in allowing it for now and tightening down later. For a related problem, please see issue ( conda-forge/conda-forge-maintenance#8 ).
There was a problem hiding this comment.
Why not add .gitattributes to all repos upon creation and enforce a particular line ending?
End-of-line conversion
While Git normally leaves file contents alone, it can be configured to normalize line endings to LF in the repository and, optionally, to convert them to CRLF when files are checked out.If you simply want to have CRLF line endings in your working directory regardless of the repository you are working with, you can set the config variable "core.autocrlf" without using any attributes.
[core]
autocrlf = true
This does not force normalization of text files, but does ensure that text files that you introduce to the repository have their line endings normalized to LF when they are added, and that files that are already normalized in the repository stay normalized.
If you want to ensure that text files that any contributor introduces to the repository have their line endings normalized, you can set the text attribute to "auto" for all files.
* text=auto
@jakirkham do you want me to change anything else at this point on this PR?
There was a problem hiding this comment.
Nope. Looks good. Thanks.
Adding .gitattributes sounds like a good idea. Maybe we should write up an issue. Alternatively, if you would like to lead the way with a PR, that would be welcome too.
|
@jakirkham made the suggested changes |
|
Thanks @goanpeca . |
Checks that there is one and only one empty line in the
meta.yamlfile@jakirkham