XML-hul code review fixes #634
Conversation
The module now correctly handles schemaLocations that define multiple namespace-to-location relationships, instead of lumping them all into the first namespace's schema location. This change also correctly reports noNamespaceSchemaLocation values as the schema's location, instead of its namespace URI.
All parameter parsing has been moved to a single location, and we now log warnings for unrecognized parameters, as well as parameter argument which include invalid URI syntax or unresolvable file paths. A misleading schema example was also removed from the default config files, which could be better illustrated in module documentation.
This prevented processing of any XML files with local schema URIs.
Includes: - Clarified, corrected and expanded documentation - Modernized code constructs (e.g. loops, auto-boxing) - Removal of unnecessary overrides and unused code - Clearer code formatting - Javadoc fixes
carlwilson
added
the
P2
|
Hi David and Carl,
|
|
Hi Jan, Yes, that's the related change that's led to your issue. So, for context, I created the sub-message resource while making the "line" and "column" strings translatable and formattable. I moved all the exception details that could vary into the sub-message with the idea that the Error ID's main message/description could/should/would benefit from remaining static, and would still describe it well enough to be documented in the wiki. I can't remember what specific benefits I might have had in mind... perhaps searchability... or possibly even just the performance benefits of static strings... but considering your use case, I see how this could make it more complicated to match specific SaxParseExceptions. And since it's a bit of a catch-all ID, the specific error detail probably warrants being kept at the main message level. So I don't see any problems with returning the SaxParseException detail to the Error's main message. We could then probably get rid of that error's sub-message entirely, since the main message would need to be dynamically generated anyway. |
|
Hi @leninoc, let me have a look at your particular issue and see if we can't do something a bit more helpful. Would a more specific ID help here at all or is it simply the main message that you can process? |
|
hi, we will discuss and i will get back to you both next week, thank you! |
|
So I think it makes no sense to have the ErrorMessage "more generic". The information is now actually semantically repetitve - you have the XML-HUL-1 in the ID which means "SAXParseException" ... and then you have "SAXParseException" again in the ErrorMessage. I'd be in favor of rolling it back. From a user perspective I expect the ID to be there to group files together based on a common class of error - and then use the ErrorMessage to take a more concrete look into the file itself. |
|
hi, yes I agree with @asciim0, rolling back makes most sense. I still would keep Submessage as they were in the module 1.5.1 where they had a value pointing to location of the issue in the file (SubMessage: Line = 422, Column = 1784) as this is important detail, although file specific. |
|
So we hear you and are currently preparing a new RC with this reverted, will be available very soon. |
|
hi @carlwilson, thank you for rolling back as requested. I tested 1.28 RC2 today and ErrorMessage now has the value/s it used to have back in 1.24, ie details from SubMessage were moved back to ErrorMessage so thats great -
But it seems that RC2 brought back the issue with repeating InfoMessages (#834)
Can you please have a look? Sorry for extra work |
|
No need to apologise, this sort of feedback gets things fixed quickly. Not too late to look. Although I'm suprised my tests missed them I think i can imagine a scenario that would cause it in the case of a reversion. Will see what's causing the duplicates and a feasible solution. Plug the regression with an appropriate test and all should be well. |
User-facing changes:
schemaLocationattributes that define multiple namespace-to-location relationshipsnoNamespaceSchemaLocationvalues as schema locations, instead of namespacesjhove.configAnd the usual code cleanup and housekeeping:
Propertyconstructs)