Joe,
Thanks for reviewing. Responses below.
On 11/05/10 13:20, [email protected] wrote:
On 11/ 5/10 07:42 AM, Dermot McCluskey wrote:
Hi,
Can I get a review for this fairly simple fix for ManifestParser bug:
http://monaco.sfbay/detail.jsf?cr=6997263
Webrev is at: http://cr.opensolaris.org/~dermot/webrev-6997263/
Thanks,
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hey Dermot,
One nit:
--------
This comment read oddly to me:
328 # If a sub-document was xincluded from a different directory
329 # from the main doc, lxml will add an attribute, eg
Perhaps/Suggestion:
If a sub-document was xincluded from a directory other than the main
doc lxml will ad an attribute, e.g.
OK. How about:
"If a sub-document is xincluded from a directory other than that where
the main
doc is located, lxml will add an attribute, e.g. ..."
One question:
-------------
I'm not a Python "re" expert but I think what you have may not work.
I've played around just now with it and it seems the "{...}" should be
"(...)":
In this context (with no previous repeatable expression) those curly braces
do not have special meaning for the regular expression - I'm actually
looking for a string which consists, in its entirety, of zero or more
characters
inside curly braces, followed by the word "base".
This works fine in all the tests I've done so far.
However, perhaps it would be clearer (and easier to maintain) if
I backslashed the curly braces to make it clearer, eg
334 base_attrib = re.compile(r'^{.*}base$')
334 base_attrib = re.compile(r'^\{.*\}base$')
py 1> re.match( r'^{.*}base$', 'http://bla hi betty base')
(note: None returned so no object is displayed here)
py 1> re.match( r'^(.*)base$', 'http://bla hi betty base')
<_sre.SRE_Match object at 0x81067a0>
(Note: This matched producing the Match object)
Also wouldn't it be better to try to be more prices, perhaps
attempting to match more in the string, perhaps adding: "^http:" and
"base=" ?
e.g.:
re.match( r'^http:(.*)base=', 'http://bla hi betty base=hi')
In most cases the string will be a http: URL, but it might possibly (?) be
a file: or other type in some instances, so I don't think I should
be too restrictive.
Is the "=...$" stripped off when walking the tree, placing "base" at
the end? If so then checking for "base=" will, of course, not work.
The "=" sign is not part of the string that I'm comparing to here - that
is only added by lxml when you print it out. (Slightly offtopic: In fact
the whole URI-inside-curly-braces bit is replaced by the string "xml:"
when you print this out. This is lxml attempting to simplify, or
canonicalize, or somehow make the output user-friendly. But it's
definitely just the URI-inside-curly-braces + "base" that I need to be
checking for at this point in the code.)
Thanks,
- Dermot
Hope this helps.
Joe
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
