Allow overriding XMLReader used in parsing

[dubinsky]

scala.xml.XML allows overriding the SAXParser used via the withSAXParser() method. Some XML parsing customizations require changing the XMLReader contained inside every SAXParser (e.g., adding an XMLFilter). This pull request introduces an additional extension point XMLLoader.reader and a method XML.withXMLReader() for such a purpose.

Also, ErrorHandler and EntityResolver configured externally are no longer wiped out before parsing the XML.

[dubinsky]

@SethTisue is there anything I can do for this to be merged? Thanks!

[SethTisue]

let's try pinging @ashawley (ping!) and if we don't hear back, I'm happy to optimistically hit "merge". the changes look fine to me, but I'm not really a good judge since I don't know this library or its code very well

would you like to be added as a maintainer in this repo? you seem to be the only one actively working on the code

[dubinsky]

let's try pinging @ashawley (ping!) and if we don't hear back, I'm happy to optimistically hit "merge". the changes look fine to me, but I'm not really a good judge since I don't know this library or its code very well

I also do not know the code that well, and would like for somebody to look over my changes - I do not want to accidentally break XML support for everybody ;)

would you like to be added as a maintainer in this repo? you seem to be the only one actively working on the code

I do have plans for a couple more pull requests, and I don't mind to be notified of other people's pull requests so that I can take a look - if that is what "maintainer" means in this context, but I have no clue how releases are done here, so you will have to continue to do those honors ;)

Thank you!

[ashawley]

I predict it is a nice enhancement. I also don't have any comment on the specific changes. They look fine.

[ashawley]

I'd only raise that it may produce binary compatibilities in 2.0. We were going to rely on the version policy plugin, that is inherited from the sbt Scala module plugin used here, to be the minder on these issues, but it was disabled, see #617.

[SethTisue]

I've submitted #641 to re-enable MiMa. Once it's merged, this PR could be rebased on top of it so we're sure it's not breaking backwards bincompat.

(It's fine to break forwards bincompat; it just means the next version will need to be 2.2.0, not 2.1.1.)

[SethTisue]

I also do not know the code that well, and would like for somebody to look over my changes - I do not want to accidentally break XML support for everybody ;)

Understood, but also, we need to strike some kind of balance here between caution and paralysis, otherwise the library becomes impossible to improve at all.

It's not the end of the world if we accidentally break something. Bugs happen. If someone notices a regression, they can report it here and downgrade to the previous version temporarily, and we'll figure out where to go from there. We should be as careful as we reasonably can, but emphasis on “reasonably”.

I do have plans for a couple more pull requests, and I don't mind to be notified of other people's pull requests so that I can take a look - if that is what "maintainer" means in this context

Well, watching the repo is sufficient to get notified of PRs.

By “maintainer” I mean the following things:

• you get the power to merge PRs, including your own, after a reasonable amount of time passes for public review
• you can also do things like label issues, close issues, edit release notes, convert issues to discussions or vice versa, etc etc etc
• you get your name in the readme :-)
• we would explicitly include you and give your opinion strong weight in decisions about releasing timing and numbering, whether to merge a controversial PR, and so forth

but I have no clue how releases are done here, so you will have to continue to do those honors ;)

sbt-ci-release (which includes sbt-dynver) makes it quite easy: you can make the release in the GitHub UI, and when GitHub Actions notices the release's tag (e.g. v2.2.0), it will automatically publish to a staging repo on Sonatype.

Then I (or someone else from the Scala org) will, on request, manually release the staging repo. (We retain this manual step for a little added security and quality control, since although the code is community-maintained the artifacts are published under the org.scala-lang.modules org, so the Scala org bears some responsibility.)

These days GitHub will even write some minimally acceptable the release notes for you, by seeing what PRs were merged since the previous release. It can be nice to make some improvements by hand.

[dubinsky]

this PR could be rebased on top of it so we're sure it's not breaking backwards bincompat

Rebased; it seems that compatibility did not break.

we need to strike some kind of balance here between caution and paralysis
merge PRs ... after a reasonable amount of time passes for public review

Please define "reasonable amount of time" ;)

would you like to be added as a maintainer in this repo?

Let's try it - I'll yell if I get overwhelmed ;)

Thanks!

[SethTisue]

Please define "reasonable amount of time" ;)

Heh... well, at the short end of “reasonable”, if something is really routine, like an sbt version bump or something, I'll typically let it sit for a couple days, just in case, or even just merge it right away.

At the long end of “reasonable”, if we're going to do something especially momentous, I might wait two or three weeks, especially if there's a key person we haven't heard from yet.

I guess for a typical PR we'd wait a week or so, something like that?

would you like to be added as a maintainer in this repo?

Let's try it - I'll yell if I get overwhelmed ;)

👍! #642