Menu

#772 Fix for reading multispace XML files with <atom>'s in other NSs

Needs_Review
closed
nobody
None
master
1
2014-07-03
2014-06-30
No

A test file is provided (eae07731), and the symptom is that it reads to many atoms if there are elements with a local name "atom" in foreign namespace (as in this test file). For this, I had to track previous modules, in order the </end> elements properly (CMLModuleStack; 1fb57bca, 40fff56c), so that I can then properly ignore other namespaces (82f0fa1d; keep in mind that within other namespaces, CML may occur again...). It also uncovered two other (small) bugs, which I fixed (5662168f, ff6f3336).

Patches available at:

https://github.com/egonw/cdk/compare/512-m-multiNamespaceCML

Discussion

  • John May

    John May - 2014-07-03

    Solution/code looks good. I still have the unit test failing in error from command line but is okay in IDE. I think is probably silly XML libs but I'm going to push and see what Jenkins makes of it.

    Other than that, a couple of minor comments:
    - resource files are best in the module being used rather then all in testdata, i'll add a patch for this
    - as correctl noted in a comment, Java Stack is bad (sync). The better one is actually Deque and ArrayDeque implementation. I'm okay with the custom stack though as I do it also :-).
    - all new classes can (and should) be package private, i'll add a patch for this
    - sb.append(stack[i]); only prints the hashCode if toString is not overridden, getClass().getSimpleName() is okay though.

     
  • John May

    John May - 2014-07-03

    applied and pushed

     
  • John May

    John May - 2014-07-03
    • status: open --> closed
     

Log in to post a comment.