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

grc_xml_generator: lxml.etree.tostring returns bytes instead of string #1815

Closed
wants to merge 1 commit into
base: next
from

Conversation

Projects
None yet
2 participants
@swap-nil7
Copy link
Contributor

swap-nil7 commented Jun 8, 2018

The outfiles must be binary written in python3.x

@marcusmueller

This comment has been minimized.

Copy link
Member

marcusmueller commented Jun 22, 2018

I'm surprised. I wasn't aware of any change in behaviour with respect to open(,"wb") in py3k. Also, the "b" traditionally doesn't do anything on Linux, just on windows. How did you come across this, and what does it fix?

@swap-nil7 swap-nil7 force-pushed the swap-nil7:xml_generator branch from d1a7c8b to 30384f5 Jun 23, 2018

@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Jun 23, 2018

@marcusmueller: I am sorry for that incorrect fix. Actually lxml.etree.tostring returns bytes instead of a string, so gr_modtool makexml raises a TypeError: write() argument must be str, not bytes while writing the grc file. The reason it works in python2.x is that str type is bytestring in that version.
Now, I have corrected the fix for the issue.

@swap-nil7 swap-nil7 changed the title grc_xml_generator: Outfiles must be binary written grc_xml_generator: lxml.etree.tostring returns bytes instead of string Jun 23, 2018

@swap-nil7 swap-nil7 force-pushed the swap-nil7:xml_generator branch from 30384f5 to 5393e8e Jun 23, 2018

@marcusmueller

This comment has been minimized.

Copy link
Member

marcusmueller commented Jun 23, 2018

Interesting! But looking at the etree.tostring:

def tostring(element, encoding=None, method=None, *,
             short_empty_elements=True):
    """Generate string representation of XML element.

    All subelements are included.  If encoding is "unicode", a string
    is returned. Otherwise a bytestring is returned.

    *element* is an Element instance, *encoding* is an optional output
    encoding defaulting to US-ASCII, *method* is an optional output which can
    be one of "xml" (default), "html", "text" or "c14n".

    Returns an (optionally) encoded string containing the XML data.

    """
    stream = io.StringIO() if encoding == 'unicode' else io.BytesIO()
    ElementTree(element).write(stream, encoding, method=method,
                               short_empty_elements=short_empty_elements)
    return stream.getvalue()

shouldn't you just be using encoding="unicode" instead of "UTF-8" instead of then re-decoding the bytes?

@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Jun 23, 2018

Actually, I tried for the version independent compatibility. But I guess, encoding = "unicode" will be better for the python3 branch. Actually, it will throw a LookUpError for python2.

@swap-nil7 swap-nil7 force-pushed the swap-nil7:xml_generator branch from 5393e8e to 8dce06c Jun 23, 2018

@swap-nil7 swap-nil7 force-pushed the swap-nil7:xml_generator branch from 8dce06c to 10242c4 Jun 28, 2018

@swap-nil7 swap-nil7 changed the base branch from python3 to next Jun 28, 2018

@marcusmueller

This comment has been minimized.

Copy link
Member

marcusmueller commented Jul 8, 2018

oh wait, I was about to merge this, but:

do we still need the xml generator in next now that that's py3+yaml?

@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Jul 8, 2018

I have replaced XML generator by YAML generator in another PR but if we wish we can just add the XML generating files and they'll work as usual due to the plug-in architecture.
Currently, imho we don't really need to merge this PR if we don't need XML generator.
Thanks for the approval though. :)

@swap-nil7

This comment has been minimized.

Copy link
Contributor

swap-nil7 commented Jul 18, 2018

Closing this since the XML generation is not required.

@swap-nil7 swap-nil7 closed this Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment