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

[MRG+1] support namespace prefix in xmliter_lxml #963

Merged
merged 1 commit into from Apr 7, 2015

Conversation

@tpeng
Copy link
Contributor

@tpeng tpeng commented Dec 1, 2014

No description provided.

@@ -2,18 +2,18 @@
from scrapy.selector import Selector


def xmliter_lxml(obj, nodename, namespace=None):
def xmliter_lxml(obj, nodename, prefix='x', namespace=None):

This comment has been minimized.

@pablohoffman

pablohoffman Dec 2, 2014
Member

why not add the prefix as the last argument (to maintain backwards compatibility with positional arguments)?

This comment has been minimized.

@tpeng

tpeng Dec 3, 2014
Author Contributor

because it's called prefix :), so i thought it's better put it before the namespace

This comment has been minimized.

@pablohoffman

pablohoffman Dec 5, 2014
Member

It doesn't sound like a strong enough reason to break backwards compatibility, I'm happy to merge if you leave prefix argument after namespace.

@tpeng tpeng force-pushed the tpeng:fix-xmliter-lxml branch from 624b27f to 82d138e Dec 15, 2014
@kmike
Copy link
Member

@kmike kmike commented Mar 13, 2015

@tpeng I wonder why are you using this code. We have some evil plans to remove it from Scrapy (see #1063), or to move it to some other place.

But I'm not familiar enough with xmliter_lxml - is it really better than scrapy.utils.iterators.xmliter? If so, does it make sense to move it to scrapy.utils.iterators? Or maybe even use it by default?

@pablohoffman
Copy link
Member

@pablohoffman pablohoffman commented Mar 17, 2015

@tpeng bump to @kmike question

@tpeng
Copy link
Contributor Author

@tpeng tpeng commented Mar 18, 2015

@kmike sorry for the delay. the original reason was i found the xmliter_lxml is memory efficient and can work with the xml i used. scrapy.utils.iterators.xmliter didn't work if the namespace is not empty.

@kmike
Copy link
Member

@kmike kmike commented Mar 19, 2015

See also: #605 which introduces a third way to parse XML iteratively in Scrapy.

@nyov
Copy link
Contributor

@nyov nyov commented Apr 3, 2015

In #1063 it was decided to keep xmliter_lxml in favor of xmliter.
Let's merge this before the relocation.
(I'll update #1134 afterwards)

@kmike kmike changed the title support namespace prefix in xmliter_lxml [MRG+1] support namespace prefix in xmliter_lxml Apr 7, 2015
@kmike
Copy link
Member

@kmike kmike commented Apr 7, 2015

+1 to merge it.

pablohoffman added a commit that referenced this pull request Apr 7, 2015
[MRG+1] support namespace prefix in xmliter_lxml
@pablohoffman pablohoffman merged commit 4b11501 into scrapy:master Apr 7, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.