add new line after reading requirements file #415
Conversation
… to list, add test cases
| @@ -94,7 +99,7 @@ def fix_requirements(f): # type: (IO[bytes]) -> int | |||
|
|
|||
| after_string = b''.join(after) | |||
|
|
|||
| if before_string == after_string: | |||
| if before_string.rstrip() == after_string.rstrip(): | |||
There was a problem hiding this comment.
I would just leave this line alone personally -- I think adding a newline to the end of the file is a good fix in the general case 👍
There was a problem hiding this comment.
will do, but as a code design question, why not to run the end_of_file_fixer at first as is?
There was a problem hiding this comment.
Some prefer to pick and choose hooks, I agree someone should probably be configuring the end-of-file-fixer as one of their default hooks -- but it's probably ok to have the behaviour happen here as well
| @@ -40,11 +40,16 @@ def __lt__(self, requirement): # type: (Requirement) -> int | |||
|
|
|||
| def fix_requirements(f): # type: (IO[bytes]) -> int | |||
| requirements = [] # type: List[Requirement] | |||
| before = tuple(f) | |||
| before = list(f) # type: List[bytes] | |||
There was a problem hiding this comment.
just an aside, the type annotation isn't required here -- mypy will properly infer that this is a list of bytes objects so generally you can leave it out -- I usually prefer to only type annotate things when mypy requires it (the inference often saves some keyboard-typing and importing)
There was a problem hiding this comment.
I am always worried from this, in case 'read' will return a single stream if its overloaded somehow:
>>> a=b'12'
>>> list(a)
[49, 50]
There was a problem hiding this comment.
I have seen the above happen when 2to3 conversion was mishandled for bytestreams.
There was a problem hiding this comment.
yeah, fortunately here the __iter__ function from IO[bytes] will give bytes (and mypy knows this)
|
One tiny thing, I edited your description to say "Resolves #..." -- that way when the PR is merged it automatically closes the issue that it is referencing Thanks again for the patch! It's greatly appreciated :) |
|
this has been released in v2.4.0! |
Resolves #414
beforevariable from tuple .This maintains all previous behaviour(s) as given by the test cases.
(my first ever OS contribution, any and all feedback is welcome)