Update dependencies #7
Conversation
mdzyuba
force-pushed
the
update_dependencies
branch
from
cd984d2 to
de6059c
Compare
| public class AndroidXmlOutputter extends XMLOutputter { | ||
| public class AndroidXmlOutputter { |
There was a problem hiding this comment.
When using an XMLOutputter, most of the printing is handled automatically. Why is this change needed? Does the outputter not support something you want to add? In any case, this does much more than updating dependencies.
There was a problem hiding this comment.
The proposed commit updates dependencies. The jdom is migrated to jdom2. The XMLOutputter is final in jdom2 - https://github.com/hunterhacker/jdom/blob/c6065f684bdff5cebf5f23b2859c5e4c89fd89db/core/src/java/org/jdom2/output/XMLOutputter.java#L150. Therefore, the updated code is using composition instead of inheritance.
There was a problem hiding this comment.
Wait, I just checked the code again. This doesn't use the outputter at all anymore. All the printing (escaping, etc) is manual now instead of mostly relying on the existing processor like before
There was a problem hiding this comment.
Do you have any suggestions on alternative options? I would like to add a few custom formatting options in following PRs. These updates will require more control over printing logic.
There was a problem hiding this comment.
I think this can be dony by inheriting from AbstractXMLOutputProcessor and passing that to the outputter
pom.xml
Outdated
| <groupId>com.bytehamster</groupId> | ||
| <artifactId>android-xml-formatter</artifactId> | ||
| <version>1.0-SNAPSHOT</version> | ||
| <version>1.1-SNAPSHOT</version> |
There was a problem hiding this comment.
We can tag the version later when all the changes are merged. I would not do that in the same PR that also adds a feature.
| <maven.compiler.source>8</maven.compiler.source> | ||
| <maven.compiler.target>8</maven.compiler.target> | ||
| <maven.compiler.source>11</maven.compiler.source> | ||
| <maven.compiler.target>11</maven.compiler.target> |
There was a problem hiding this comment.
Is there a specific reason for this? Any Java 11 features you need?
There was a problem hiding this comment.
- JUnit Jupiter 5.11.4 (requires Java 11+)
- Spotless Maven Plugin 2.44.0 (requires Java 11+)
CHANGELOG.md
Outdated
| @@ -0,0 +1,38 @@ | |||
| # Changelog | |||
There was a problem hiding this comment.
I would rather manage the changelog in the github releases, not in a dedicated file
mdzyuba
force-pushed
the
update_dependencies
branch
3 times, most recently
from
4a44e31 to
ca61218
Compare
|
I've updated the commit and addressed the comments. |
|
Could you rebase/merge main please? Currently this PR has merge conflicts |
Dependencies updated: - Java: 8 → 11 - JDOM: 1.1.3 → JDOM2 2.0.6.1 - Commons CLI: 1.4 → 1.9.0 - Commons Lang: 3.12.0 → 3.17.0 Plugins updated: - Maven Assembly: 3.1.1 → 3.7.1 - Maven Surefire: 3.2.5 → 3.5.2 Java 11 requirement: The following dependencies require Java 11 as minimum: - JUnit Jupiter 5.11.4 (requires Java 11+) - Spotless Maven Plugin 2.44.0 (requires Java 11+) Documentation: - Updated README Code changes: - Refactored AndroidXmlOutputter to use composition instead of inheritance since JDOM2's XMLOutputter cannot be subclassed.
mdzyuba
force-pushed
the
update_dependencies
branch
from
ca61218 to
ddc160e
Compare
|
Rebased. |
2 participants
This PR depends on the previous PR #5 that adds tests. I will rebase it once the PR #5 is approved and merged.