Added support for load_map_from_string in BaseWMSFactory.loadXML method (2)

[p0cisk]

Hi,
This pull request adds possibility to load mapnik XML configuration from string using load_map_from_string function. There is a check if 'xmlfile' is existing file (os.path.isfile) - if not 'load_map_from_string' will be used.
This is second pull request after #26 - earlier I've add wrong checkig - sorry for that.

Regards
Piotr Pociask

[manelclos]

Hi Piotr,

Having this would be great for testing. I'd prefer to have this as a different parameter, as both are strings and current one is named xmlfile. Would you mind modifying it to be something like:

def loadXML(self, xmlfile, strict=False, xmlstring=None):

And then xmlstring != None will use it and ignore xmlfile.

What do you think?

[p0cisk]

I doesn't want to adding new parameter in loadXML definition because I want to interfere in the code as little as possible but I'm OK with your suggestion. What do you think to modified it like this:

def loadXML(self, xmlfile=None, strict=False, xmlstring=None):

so user don't need to set xmlfile if he use only xmlstring? It shouldn't also break any earlier compatibility.

[manelclos]

looks good, can you also create a test so the new code gets covered?

[p0cisk]

Hi
I've changed loadXML definition. load_map_from_string can also have base_path parameter that is useful for supporting relative paths so I also added this to loadXML. If user loads data from files (like Shapefiles) it is good to have this parameter setted.

[manelclos]

Hi, looks good. One final petition before merge. We should check xmlfile and xmlstring are not both empty, in that case we should raise and Exception explaining one of the two options is mandatory (since we changed xmlfile from mandatory to optional).

[p0cisk]

Is this should rise OGCException, ServerConfigurationError or I should add new exception type?

[manelclos]

I'd say this is a configuration error.
On 29 Jun, 2014 9:26 PM, "Piotr Pociask" notifications@github.com wrote:

Is this should rise OGCException, ServerConfigurationError or I should add
new exception type?

—
Reply to this email directly or view it on GitHub
#27 (comment).

[p0cisk]

I've added checking xmlfile and xmlstring variables, test for checking exception and I've clear some unnecessary imports in previous test.

[manelclos]

Perfect! Thanks!