circleci.test icon indicating copy to clipboard operation
circleci.test copied to clipboard

Port to data.xml v0.2.0-alpha5

Open joodie opened this issue 6 years ago • 5 comments

Necessary changes to support alpha version of clojure.data.xml, which supports clojurescript and xml namespaces.

joodie avatar Mar 06 '18 12:03 joodie

Since this is backward-incompatible with the previous data.xml, I'm not sure what the right thing to do is here. We needed to run with the newer, alpha-release of data.xml since that's what we have in our codebase, so we deployed our own version of this branch.

joodie avatar Mar 06 '18 12:03 joodie

Thanks for the PR! Would you mind undoing the :require/:import re-ordering, and the whitespace-only formatting changes?

I am hesitant to switch to an alpha release of data.xml since that forces consumers of circleci.test to switch as well. I would like to wait until the new API for data.xml is declared stable before I could consider forcing that change on others.

My current suspicion is that the correct approach is to switch to javax.xml for XML processing and drop the clojure.data.xml dependency altogether.

gordonsyme avatar Mar 06 '18 13:03 gordonsyme

I updated the code to do only the minimal changes required to support data.xml 0.2.0-alpha.

I'm note sure either what's the best strategy wrt versions. Possibly an alpha release would work. I think it should be possible to make the code work with both the stable and the alpha versions; the easiest route would be to drop all the element? tests & assertions from this branch (and the code for element? could also be lifted, it's only 3 lines or so and should work with the stable version too)

joodie avatar Mar 06 '18 13:03 joodie

@gordonsyme I just added a commit that removes the hard dependency on data.xml alpha versions, and some tests to show that this works on both the stable and alpha versions, so it's usable by everybody on at least current stable and latest alpha.

joodie avatar Mar 06 '18 14:03 joodie

Lazy dev question :wink: Does this make it ClojureScript compatible?

Sorry for the noise: checked the code and it is Clojure only for now.

arichiardi avatar Sep 28 '18 05:09 arichiardi