[ARCHETYPE-584] fix indentation in root pom.xml #35

newur · 2020-01-05T06:59:21Z

Problem: linebreaks + indentions (whitespaces) are read as actual document nodes. This causes problems in combination with

tr.setOutputProperty( OutputKeys.INDENT, "yes" );
tr.setOutputProperty( "{http://xml.apache.org/xslt}indent-amount", "2" );

Removed the .normalizeWhitespace() in tests, because this shadowed the actual bug. Also split one test into multiple tests. Test content remains the same.

Had to add the maven-4.0.0.xsd, not sure if this is desired.

Closes #35

softdays · 2020-02-25T07:46:52Z

Any chance to get this PR merged?

Closes #35

elharo · 2020-04-16T18:50:47Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


-        out = new StringWriter();
+        StringWriter out = new StringWriter();


Looks like this could be a field that doesn't need to be initialized in each method.

Moved to setup method.

elharo · 2020-04-16T18:53:10Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java

@@ -61,6 +61,17 @@
 public class ArchetypeTest
    extends PlexusTestCase
 {
+    private static final String XML_VERSION_1_0 =


splitting out these constants makes the tests harder for me to read. I prefer having all the expected content directly in each method, even at the expense of some duplication.

Funny how different people read code. I could barley make any sense out of the initial version. Is revert in the new commit.

newur · 2020-05-11T22:39:48Z

Any progress on this PR?

elharo · 2020-05-11T23:42:55Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


    private OldArchetype archetype;

+    private StringWriter out;


initialize this here

It is initialized in the setup method. If I init it here it would be done twice. If I remove it from the setup the tests would share state.

Or do I miss something here?

Btw if any maintainer really wants it to be changed, please fell free to do so. Whatever works best, I am fine with it.

Yes, you're missing something. Tests do not share instance variables. Every test is run with a new object.

elharo · 2020-05-11T23:43:10Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


-        StringWriter out = new StringWriter();
+        out = new StringWriter();


you don't need to initialize this in the test

elharo · 2020-05-11T23:43:16Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


-        StringWriter out = new StringWriter();
+        out = new StringWriter();


you don't need to initialize this in the test

elharo · 2020-05-11T23:43:25Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


-        StringWriter out = new StringWriter();
+        out = new StringWriter();


you don't need to initialize this in the test

elharo · 2020-05-11T23:43:29Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


-        StringWriter out = new StringWriter();
+        out = new StringWriter();


you don't need to initialize this in the test

elharo · 2020-05-11T23:43:36Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


-        StringWriter out = new StringWriter();
+        out = new StringWriter();


you don't need to initialize this in the test

elharo · 2020-05-11T23:43:44Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


-        StringWriter out = new StringWriter();
+        out = new StringWriter();


you don't need to initialize this in the test

You are absolutly right. Fixed all occurances.

elharo · 2020-05-11T23:44:28Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java

@@ -453,5 +489,6 @@ protected void setUp()
    {
        super.setUp();
        archetype = (OldArchetype) lookup( OldArchetype.ROLE );
+        out = new StringWriter();


setUp method usually goes near the top

I kept the method where it was before my patch. I think that keeps reviewing changes easier.
While I personally also prefer it at the top, there is another class which has it after the test, so I assume this is somehow an intended style.

newur · 2020-05-12T15:09:17Z

Updated the branch

hboutemy · 2020-06-05T05:56:18Z

@elharo is this ok now to merge, or do you think something additional is required?

elharo

Just one nit remaining but atm I don't see anything blocking

elharo · 2020-06-05T11:01:17Z

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java


    private OldArchetype archetype;

+    private StringWriter out;


hboutemy · 2020-06-08T20:39:21Z

can't merge, there are 4 ITs failing: https://builds.apache.org/blue/organizations/jenkins/maven-box%2Fmaven-archetype/detail/ARCHETYPE-584/14/pipeline :(

elharo

IT failures look real. I'm guessing that the golden data needs to be updated to match the output after this PR:

[ERROR] The following builds failed:
[ERROR] * ARCHETYPE-241_filter-directory/pom.xml
[ERROR] * ARCHETYPE-536_groovy-grape/pom.xml
[ERROR] * roundtrip-multi/pom.xml
[ERROR] * build-and-run-its/pom.xml

jorgheymans · 2021-06-30T09:14:49Z

IT failures look real. I'm guessing that the golden data needs to be updated to match the output after this PR:

How does one update the golden data then ? This PR has gone stale but seems really only this last hurdle is missing.

newur · 2021-07-03T07:30:09Z

Wow, this branch/bug is still around. I totally forgot about it.

After playing around with the integration tests a bit, one problem was to load the .xsd from the packaged .jar (which works differently from reading from a single file). Solving the loading from the jar seems to fix the ITs.

Let's see what comes up next or if this long journey reaches its happy end. :)

elharo · 2021-07-03T11:39:55Z

archetype-common/src/main/java/org/apache/maven/archetype/common/util/PomUtils.java

@@ -79,6 +83,13 @@ public static boolean addNewModule( String artifactId, Reader fileReader, Writer
        dbf.setXIncludeAware( false );
        dbf.setExpandEntityReferences( false );

+        InputStream inputStream = PomUtils.class.getClassLoader().getResourceAsStream( "maven-4.0.0.xsd" );


What is the purpose of loading this schema?

Please see my minimal, reproducible example here https://issues.apache.org/jira/browse/ARCHETYPE-584?focusedCommentId=17008264&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17008264

I investigated this 1.5 years ago and cannot remember the details why it only worked this way. However, I am pretty sure that I did not dive into the implementation details of DocumentBuilderFactory, which is fed with the schema, so not sure if I would have been able to explain it very deep back then.

My guess is that you don't need the schema to fix this bug. In fact, you probably only need setIgnoringElementContentWhitespace though setNamespaceAware( true ) is always a good idea.

Please check the example I provided. When you run it, you will see that without the schema the output is not as expected.

elharo · 2021-07-03T11:40:58Z

archetype-common/src/main/resources/maven-4.0.0.xsd

@@ -0,0 +1,2246 @@
+<?xml version="1.0"?>


strange that this has to be copied into this project. I'd expect it to be loaded from maven core, if at all.

Feel free to point me to location of the xsd in maven core or any other way to load it and I happily update the PR.

archetype-common/src/test/java/org/apache/maven/archetype/old/ArchetypeTest.java

newur · 2021-08-02T23:17:46Z

I committed a new attempt to get this merged. Since I do not understand which solution is acceptable, I made an unclean commit (code in comments, no proper exception handling).
Would be nice to have some feedback, if this solution has any chance to get accepted before I clean it up.

kwin · 2024-01-30T11:45:08Z

I think this is superseded by #128

newur · 2024-02-04T14:16:39Z

I think so too and would love to try it out @kwin. Unfortunately maven-archetype-plugin version 3.3.0 is not released, yet.

Any estimated time when the release will happen? Or any way to test this, besides building the plugin locally?

Ruwen Reddig added 2 commits January 5, 2020 07:49

ARCHETYPE-584 fix indention in root pom.xml

95fc777

ARCHETYPE-584 nicer formatting

4bf79b9

asfgit pushed a commit that referenced this pull request Feb 16, 2020

ARCHETYPE-584 fix indention in root pom.xml

5b4deaa

Closes #35

asfgit pushed a commit that referenced this pull request Feb 16, 2020

[ARCHETYPE-584] fix indention in root pom.xml

7032f4f

Closes #35

asfgit pushed a commit that referenced this pull request Feb 16, 2020

[ARCHETYPE-584] fix indention in root pom.xml

2517144

Closes #35

asfgit pushed a commit that referenced this pull request Feb 16, 2020

[ARCHETYPE-584] fix indention in root pom.xml

4a3d5ef

Closes #35

asfgit pushed a commit that referenced this pull request Mar 8, 2020

[ARCHETYPE-584] fix indention in root pom.xml

a8c345b

Closes #35

elharo reviewed Apr 16, 2020

View reviewed changes

ARCHETYPE-584 refactored test according to review comments

6f012d7

elharo reviewed May 11, 2020

View reviewed changes

removed double initialization

6e0fe84

elharo reviewed Jun 5, 2020

View reviewed changes

ARCHETYPE-584 clean up

b984e59

elharo approved these changes Jun 5, 2020

View reviewed changes

elharo requested changes Jun 8, 2020

View reviewed changes

elharo changed the title ~~ARCHETYPE-584 fix indention in root pom.xml~~ [ARCHETYPE-584] fix indentation in root pom.xml Jul 14, 2020

fix loading a file from a .jar file

12d2049

newur requested a review from elharo July 3, 2021 07:32

elharo reviewed Jul 3, 2021

View reviewed changes

newur mentioned this pull request Jul 8, 2021

[ARCHETYPE-584] don't normalize whitespace in tests so we can test for whitespace #65

Draft

remove empty nodes via XPath

8e4a636

newur requested a review from elharo August 2, 2021 23:15

wborn mentioned this pull request Nov 5, 2022

Script to create new binding skeleton: problem with file bundles/pom.xml openhab/openhab-addons#13405

Open


		out = new StringWriter();
		StringWriter out = new StringWriter();

[ARCHETYPE-584] fix indentation in root pom.xml #35

Are you sure you want to change the base?

[ARCHETYPE-584] fix indentation in root pom.xml #35

Conversation

newur commented Jan 5, 2020

softdays commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newur commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newur Jun 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newur commented May 12, 2020

hboutemy commented Jun 5, 2020

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hboutemy commented Jun 8, 2020

elharo left a comment

Choose a reason for hiding this comment

jorgheymans commented Jun 30, 2021

newur commented Jul 3, 2021 • edited

Choose a reason for hiding this comment

newur Jul 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newur Jul 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newur commented Aug 2, 2021

kwin commented Jan 30, 2024

newur commented Feb 4, 2024 • edited

newur Jun 5, 2020 •

edited

newur commented Jul 3, 2021 •

edited

newur Jul 3, 2021 •

edited

newur Jul 3, 2021 •

edited

newur commented Feb 4, 2024 •

edited