-
Notifications
You must be signed in to change notification settings - Fork 134
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 tag value writer #517
Fix tag value writer #517
Conversation
Signed-off-by: Meret Behrens <[email protected]>
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.
Thanks for the fix, I have just some stylistic remarks.
m = mock_open() | ||
with patch("{}.open".format(__name__), m, create=True): | ||
with open("foo", "w") as h: | ||
write_creation_info(creation_info, h) |
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.
I get that they are throw-away variables, but can we give m
and h
slightly more descriptive names?
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
m.assert_called_once_with("foo", "w") | ||
handle = m() | ||
handle.write.assert_has_calls( | ||
expected_calls) |
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.
Unnecessary line break
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.
fixed
call("Created: 2022-03-10T00:00:00Z\n")])]) | ||
def test_creation_info_writer(creation_info, expected_calls): | ||
m = mock_open() | ||
with patch("{}.open".format(__name__), m, create=True): |
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.
Why not use an f-string here?
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.
fixed, I just copied the code from test_package_writer.py
. I would suggest to change the formatted string there when fixing #394 is fixed, okay?
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.
I'd prefer fixing it now so that next time it is not copied from there again ;)
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
ffd5fb3
to
63797e2
Compare
Signed-off-by: Meret Behrens <[email protected]>
… names Signed-off-by: Meret Behrens <[email protected]>
63797e2
to
b48ddf2
Compare
Due to the type conversion before entering the
set_value
method aNone
value forLicenseListVersion
was written to the outptut file. Thef-string
calls the implementedstr
-conversion for the instance ofVersion
, so we don't need to do this conversion explicitly.To improve the test coverage here, I also added a test.