Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to data.xml v0.2.0-alpha5 #23

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Port to data.xml v0.2.0-alpha5 #23

wants to merge 2 commits into from

Conversation

joodie
Copy link

@joodie joodie commented Mar 6, 2018

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

@joodie
Copy link
Author

joodie commented Mar 6, 2018

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.

@gordonsyme
Copy link
Member

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.

Minimal changes to support data.xml 0.2.0-alpha5 release. This is
backwards incompatible with data.xml previous to 0.2.0.
@joodie
Copy link
Author

joodie commented Mar 6, 2018

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)

Includes configuration to test that reporter works with current stable
and latest data.xml
@joodie
Copy link
Author

joodie commented Mar 6, 2018

@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.

@arichiardi
Copy link

arichiardi commented Sep 28, 2018

Lazy dev question 😉 Does this make it ClojureScript compatible?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants