Review: Approve
--
https://code.launchpad.net/~zorba-coders/zorba/canonicalize-core-fixed/+merge/142394
Your team Zorba Coders is subscribed to branch lp:zorba.
--
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe :
The attempt to merge lp:~zorba-coders/zorba/canonicalize-core-fixed into
lp:zorba failed. Below is the output from the failed tests.
CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275
(message):
Validation queue job canonicalize-core-fixed-2013-03-06T08-06-58.258Z
Review: Needs Fixing
I just realized that the set of options for canonicalization (as proposed by
Matthias) are overlapping with the options that already exist for XML parsing.
But the current implementation adds an entirely new set of options. This looks
pretty silly, and is a waste of code.
My proposal:
1. Add a new method: int store::LoadProperties::toLibXmlOptions()
The implementation of this method should be copied from
zorba::simplestore::XmlLoader::applyLoadOptions() (minus the final call to
xmlCtxtUseOptions() ), and applyLoadOptions() should of course be refactored to
Review: Approve
I approve conditionally. There are still some things that require fixing:
1. The following from the diff doesn't make sense.
547 +zstring lNodeName = child-getNodeName()-getLocalName();
548 +std::transform(
549 + lNodeName.begin(), lNodeName.end(),
550
I approve conditionally. There are still some things that require fixing:
1. If the options are validated using a schema, there is no need to transform
them to
lower case because the schema is case-sensitive. Also, the code should
validate the namespace of the node, not only the local name.
The attempt to merge lp:~zorba-coders/zorba/canonicalize-core-fixed into
lp:zorba failed. Below is the output from the failed tests.
CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275
(message):
Validation queue job canonicalize-core-fixed-2013-03-05T04-20-03.545Z
The attempt to merge lp:~zorba-coders/zorba/canonicalize-core-fixed into
lp:zorba failed. Below is the output from the failed tests.
CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275
(message):
Validation queue job canonicalize-core-fixed-2013-03-05T06-14-06.536Z
Hi the changes for the private function are already there, when you said
something else was missing I thought you meant
The example doesn't mention that the options are in a namespace? According to
the schema they should be. According to the implementation, the namespace
isn't considered.
- Which options are enabled by default?
The only option available as default is XML_PARSE_NOERROR this way zorba can
manage the error messages and no extra message are shown, but this option is
not optional.
--
- There should be a private canonicalize function that does the schema
validation if the input is not validated similarly to most other modules.
Matthias: could you give an example from other modules of what you're referring
to here?
--
- There should be a private canonicalize function that does the schema
validation if the input is not validated similarly to most other modules.
Matthias: could you give an example from other modules of what you're
referring to here?
zorba_modules/html/src/html.xq:119
--
Juan has added the canonicalize#2 function as suggested.
I have pushed a couple additional comment additions, plus a preventative fix
that ensures this change to the XML module won't interfere with runtime
generation.
Matthias, please re-review.
--
Review: Needs Fixing
- Why is the following change necessary?
- -q import module namespace file = 'http://expath.org/ns/file';
file:exists( 'a non existant file' )
+ -q import module namespace file = 'http://expath.org/ns/file'; import
module namespace x =
Regarding the GenZorbaRuntime.cmake change: I noticed when I merged this change
locally *into an already-working build* that the build started failing. Turns
out to be because GenZorbaRuntime implicitly depends on the XML module, and
once the new xml.xq file was installed in URI_PATH, you
Review: Needs Fixing
There should be a canonicalize#2 function that allows to specify options.
This should at least allow for the following libxml2 options.
XML_PARSE_NOENT = 2 : substitute entities
XML_PARSE_NOBLANKS = 256 : remove blank nodes
XML_PARSE_NONET = 2048 : Forbid network
Sorry I had to remove the previous branch proposed for merge because of to much
of a mess between merges that it seemed some data was lost.
This branch contains all the changes Matthias previously mentioned
including the two last
Fixed the lost of changes made because of wrong merge with trunk
Review: Approve
--
https://code.launchpad.net/~zorba-coders/zorba/canonicalize-core-fixed/+merge/142394
Your team Zorba Coders is subscribed to branch lp:zorba.
--
Mailing list: https://launchpad.net/~zorba-coders
Post to : zorba-coders@lists.launchpad.net
Unsubscribe :
18 matches
Mail list logo
