Skip to content
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

Py3: Fix ToASCII method #334

Closed
wants to merge 1 commit into from
Closed

Py3: Fix ToASCII method #334

wants to merge 1 commit into from

Conversation

MartinBasti
Copy link
Contributor

@MartinBasti MartinBasti commented Dec 14, 2016

in Py2 to_text method returns Py2 non-unicode string, but in Py3 to_text method
returns Py3 default (unicode) string. So only in Py2 we have to decode
str to unicode.

https://fedorahosted.org/freeipa/ticket/5935

Edit: ticket number

res = self.to_text()
if six.PY2:
return res.decode('ascii') # must be unicode string in Python 2
return res
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Python 3 the ToASCII method can theoretically return non-ASCII text.

if six.PY2:
    return res.decode('ascii') # must be unicode string in Python 2
else:
    res.encode('ascii')  # ensure it's only ASCII
    return res

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but to_text() returns punycoded values, escaped values as octal numbers, and ascii 32<characters<=127. So I don't think that string can contain something else than ASCII characters < 128

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but anyway I will add there the check you mentioned

@tiran
Copy link
Member

tiran commented Dec 14, 2016

I left a comment

# 1.13: python-dns URI record support
BuildRequires: python-dns >= 1.13
# 1.15: python-dns PY3 support (many improvements)
BuildRequires: python-dns >= 1.15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't bump BuildRequires unless it is actually required for the build. Doing so does not add any value, only the chore of having to maintain additional packages in our build roots for no reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is conditional just for lint checking, IMO we may need to test with lint the same packages as are in Requires

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not guarantee that the package checked during build will be the same as the package used in runtime in any way. But given that an API we use was changed in 1.15, the bump is in fact required.

@MartinBasti
Copy link
Contributor Author

I reworked commit:

  • keep python-dns 1.15 in build requires at it changed return type of to_text function
  • removed .encode() check

in Py2 to_text method returns Py2 non-unicode string, but in Py3 to_text method
returns Py3 default (unicode) string. So only in Py2 we have to decode
str to unicode.

https://fedorahosted.org/freeipa/ticket/5935
@tiran
Copy link
Member

tiran commented Jan 4, 2017

I have kicked Travis. Let's see if the test is passing now.

@MartinBasti
Copy link
Contributor Author

bump for review

@tiran tiran added the ack Pull Request approved, can be merged label Jan 6, 2017
@MartinBasti
Copy link
Contributor Author

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Jan 6, 2017
@MartinBasti MartinBasti closed this Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants