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
encode/decode UTF-8 in POMs #44
Conversation
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, excellent work!
Can you please consider my review notes.
jip/util.py
Outdated
@@ -71,6 +71,7 @@ def download_string(url): | |||
import requests | |||
try: | |||
response = requests.get(url, headers={ 'User-Agent': JIP_USER_AGENT}) | |||
response.encoding = 'utf-8' |
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.
This doesn't seem to be required. I don't think it makes any sense to modify the response that comes from the server.
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.
Yes, it's a dubious operation, but Maven repository gives UTF-8 encoded files without content encoding header, so the 'requests' incorrectly assigns the 'iso-8859-1' encoding.
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.
Ok. But it doesn't seem to matter what 'requests' assigns. I've tried the code without setting the encoding attribute and it works. I think it is just metadata about the response that you can query if you want. But it doesn't affect how the data is interpreted.
test/install_test.py
Outdated
@@ -18,6 +18,11 @@ def testResolve(self): | |||
artifacts = _resolve_artifacts([junit]) | |||
self.assertEqual(len(artifacts), 1) | |||
|
|||
def testResolve2(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.
Can you please give the test method a more meaningful name? Like testResolveArtifactWithUmlautsInPom or so?
test/install_test.py
Outdated
@@ -18,6 +18,11 @@ def testResolve(self): | |||
artifacts = _resolve_artifacts([junit]) | |||
self.assertEqual(len(artifacts), 1) | |||
|
|||
def testResolve2(self): | |||
junit = Artifact('de.l3s.boilerpipe', 'boilerpipe', '1.1.0') |
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.
Please rename the variable junit
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, perfect!
Issue #43
Works for me. Tests are passed.