-
Notifications
You must be signed in to change notification settings - Fork 5
Annex: accept new line in pointer detection logic #44
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
base: master
Are you sure you want to change the base?
Conversation
qa-cea
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you encountered this problem often? Pointers generated by Rift should not contain line breaks.
Personally never! But @valeriyoann did and he thinks Rift should permit this. |
lib/rift/Annex.py
Outdated
| if meta.st_size in (32, 64): | ||
| # MD5 (32) or SHA3 256 (64). Also accept one or two more bytes to accept | ||
| # trailing new line and carriage return characters. | ||
| if meta.st_size in (32, 33, 34, 64, 65, 66): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest: I think it'd be easier to do it in reverse
with open:
identifier = ...
if identifier.size() not in (32, 64):
return False
return all(...)
What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it makes the logic more simple. Done in 3137f79.
| temp_file = make_temp_file(correct_identifier) | ||
| self.assertTrue(Annex.is_pointer(temp_file.name)) | ||
|
|
||
| def test_is_pointer_valid_identifier_with_carriage_return(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest: add the same test for spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 3137f79.
Also add test with trailing whitespace.
fc54c3c to
3137f79
Compare
No description provided.