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

Make it possible to temporarily override output method when calling _IncrementalWriter.write #210

Merged
merged 4 commits into from Dec 4, 2016

Conversation

Projects
None yet
2 participants
@plq
Contributor

plq commented Nov 22, 2016

Behold:

>>> from lxml import html, etree
>>> from lxml.builder import E
>>> html.tostring(E.foo(selected="bar"))
'<foo selected></foo>'
>>> etree.tostring(E.foo(selected="bar"))
'<foo selected="bar"/>'

With custom elements, HTML is no more a tag soup of predefined tag names. As can be seen in: https://user-content-dot-custom-elements.appspot.com/PolymerElements/paper-dropdown-menu/v1.4.2/paper-dropdown-menu/demo/index.html we may need to have the value of the selected attribute.

My solution to this is the ability to override the output method temporarily in write calls.

Stefan, If the patch is as simple as this, could you advise about the actual value to pass to write() when overriding method?

Thanks & best regards

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Nov 26, 2016

Member

See a bit further up in the file, there is a helper function for this:

    c_method = _findOutputMethod(method)

Seems like a nice feature. I'd like to see a bit of documentation added, though, and a test for what the different output methods do in this context.

Member

scoder commented Nov 26, 2016

See a bit further up in the file, there is a helper function for this:

    c_method = _findOutputMethod(method)

Seems like a nice feature. I'd like to see a bit of documentation added, though, and a test for what the different output methods do in this context.

@plq

This comment has been minimized.

Show comment
Hide comment
@plq

plq Nov 29, 2016

Contributor

Just added tests. If you like it so far, I can add some docs as well. Where do you think we should talk about this?

Contributor

plq commented Nov 29, 2016

Just added tests. If you like it so far, I can add some docs as well. Where do you think we should talk about this?

@plq

This comment has been minimized.

Show comment
Hide comment
@plq

plq Nov 29, 2016

Contributor

According to https://developers.google.com/web/fundamentals/getting-started/primers/customelements custom elements must contain a dash in tag names. Shold we maybe make this automatic for elements with dash in them?

Contributor

plq commented Nov 29, 2016

According to https://developers.google.com/web/fundamentals/getting-started/primers/customelements custom elements must contain a dash in tag names. Shold we maybe make this automatic for elements with dash in them?

@plq

This comment has been minimized.

Show comment
Hide comment
@plq

plq Nov 29, 2016

Contributor

Hmmm, that'd be a bad idea. .write() call would have to go through the entire tree to see whether there are any tags with dash in them. never mind :)

Contributor

plq commented Nov 29, 2016

Hmmm, that'd be a bad idea. .write() call would have to go through the entire tree to see whether there are any tags with dash in them. never mind :)

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Dec 1, 2016

Member

Looks good so far. I think that extending the docstring would be enough.

The problem with the HTML attributes that you showed is because the serialisation is done manually directly in the writer, and it doesn't actually know about HTML. I agree that it should be fixed, but I'm not sure how exactly this should be done.

Member

scoder commented Dec 1, 2016

Looks good so far. I think that extending the docstring would be enough.

The problem with the HTML attributes that you showed is because the serialisation is done manually directly in the writer, and it doesn't actually know about HTML. I agree that it should be fixed, but I'm not sure how exactly this should be done.

@plq

This comment has been minimized.

Show comment
Hide comment
@plq

plq Dec 2, 2016

Contributor

Looks good so far. I think that extending the docstring would be enough.

Done. Anything else?

The problem with the HTML attributes that you showed is because the serialisation is done manually directly in the writer, and it doesn't actually know about HTML.

I think it'd be possible to add the method argument to element() function, subsequently add method information to _FileWriterElement class which in turn could use that info to generate the correct byte stream using two sets for void elements and void attributes, just like libxml does.

Whether we should add missing void elements to that list and introduce another incompatibility with what libxml does is a decision you will need to make.

Contributor

plq commented Dec 2, 2016

Looks good so far. I think that extending the docstring would be enough.

Done. Anything else?

The problem with the HTML attributes that you showed is because the serialisation is done manually directly in the writer, and it doesn't actually know about HTML.

I think it'd be possible to add the method argument to element() function, subsequently add method information to _FileWriterElement class which in turn could use that info to generate the correct byte stream using two sets for void elements and void attributes, just like libxml does.

Whether we should add missing void elements to that list and introduce another incompatibility with what libxml does is a decision you will need to make.

@plq

This comment has been minimized.

Show comment
Hide comment
@plq

plq Dec 2, 2016

Contributor

I think that's a discussion for another issue.

Contributor

plq commented Dec 2, 2016

I think that's a discussion for another issue.

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Dec 4, 2016

Member

Agreed. Thanks for your contribution.

Member

scoder commented Dec 4, 2016

Agreed. Thanks for your contribution.

@scoder scoder merged commit 4b8e177 into lxml:master Dec 4, 2016

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