You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/12/15 11:33:55 UTC
[GitHub] [maven] pzygielo opened a new pull request, #914: Generate or implement some `toString`s
pzygielo opened a new pull request, #914:
URL: https://github.com/apache/maven/pull/914
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
----
If something decides to dump some build/session details, like `maven-bundle-plugin` with `--debug` does, currently it produces:
```
[DEBUG] BND Instructions:
project.build.mailingLists=[org.apache.maven.model.MailingList@16e91d6f, org.apache.maven.model.MailingList@57104caa]
project.licenses=[org.apache.maven.model.License@416855dc, org.apache.maven.model.License@1f342afb]
project.build.scm=org.apache.maven.model.Scm@5d4369c5
```
I propose to change that to something more useful, like
```
[DEBUG] BND Instructions:
project.build.mailingLists=[name \= '...']
project.licenses=[License(name\=Eclipse Public License v. 2.0, url\=https\://www.eclipse.org/org/documents/epl-2.0/EPL-2.0.txt), License(name\=GNU General Public License, version 2 with the GNU Classpath Exception, url\=https\://www.gnu.org/software/classpath/license.html)]
project.build.scm=connection \= '...'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] pzygielo commented on pull request #914: Generate or implement some `toString`s
Posted by GitBox <gi...@apache.org>.
pzygielo commented on PR #914:
URL: https://github.com/apache/maven/pull/914#issuecomment-1352982396
May I ask for review, please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #914: Generate or implement some `toString`s
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #914:
URL: https://github.com/apache/maven/pull/914#discussion_r1049989101
##########
maven-model/src/test/java/org/apache/maven/model/BuildTest.java:
##########
@@ -53,4 +53,12 @@ public void testToStringNullSafe()
assertNotNull( new Build().toString() );
}
+ public void testToStringNotNonsense()
+ {
+ Build build = new Build();
+
+ String s = build.toString();
+
+ assert "Build(super=BuildBase(super=PluginConfiguration(super=PluginContainer())))".equals( s ) : s;
Review Comment:
That's a very verbose and not really useful `toString()` ...
##########
maven-model/src/test/java/org/apache/maven/model/OrganizationTest.java:
##########
@@ -53,4 +53,36 @@ public void testToStringNullSafe()
assertNotNull( new Organization().toString() );
}
+
+ public void testToStringNotNonsense11()
+ {
+ Organization org = new Organization();
+ org.setName( "Testing Maven Unit" );
+ org.setUrl( "https://maven.localdomain" );
+
+ assertEquals( "name = 'Testing Maven Unit'\nurl = 'https://maven.localdomain'", org.toString() );
Review Comment:
That looks wrong. The `toString()` result should at least have the simple class name in it.
Same for `MailingList`, `Scm` and others...
##########
maven-model/src/test/java/org/apache/maven/model/OrganizationTest.java:
##########
@@ -53,4 +53,36 @@ public void testToStringNullSafe()
assertNotNull( new Organization().toString() );
}
+
+ public void testToStringNotNonsense11()
+ {
+ Organization org = new Organization();
+ org.setName( "Testing Maven Unit" );
+ org.setUrl( "https://maven.localdomain" );
+
+ assertEquals( "name = 'Testing Maven Unit'\nurl = 'https://maven.localdomain'", org.toString() );
+ }
+
+ public void testToStringNotNonsense10()
+ {
+ Organization org = new Organization();
+ org.setName( "Testing Maven Unit" );
+
+ assertEquals( "name = 'Testing Maven Unit'\nurl = 'null'", org.toString() );
Review Comment:
The `'null'` definitely looks wrong. If quotes are added when there is a string, it should be `url = null` when the url is actually `null` (with no quotes).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #914: Implement some `toString`s
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #914:
URL: https://github.com/apache/maven/pull/914#issuecomment-1356817229
Looking at ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #914: Implement some `toString`s
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #914:
URL: https://github.com/apache/maven/pull/914#issuecomment-1356820610
Merged with 6c53b28ecc84f0f90d5417f199d99b20334f26d7.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] pzygielo commented on pull request #914: Generate or implement some `toString`s
Posted by GitBox <gi...@apache.org>.
pzygielo commented on PR #914:
URL: https://github.com/apache/maven/pull/914#issuecomment-1353747359
Replaced generating methods by modello with manual implementations.
Still don't know what to put for `Build` though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o closed pull request #914: Implement some `toString`s
Posted by GitBox <gi...@apache.org>.
michael-o closed pull request #914: Implement some `toString`s
URL: https://github.com/apache/maven/pull/914
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] pzygielo commented on pull request #914: Implement some `toString`s
Posted by GitBox <gi...@apache.org>.
pzygielo commented on PR #914:
URL: https://github.com/apache/maven/pull/914#issuecomment-1356813946
> > @michael-o - [[HEADS UP] Maven 3.8.7](https://lists.apache.org/thread/7p81o29z56k1kw1tc3k5j1t5fg4rsr2d) - may I interest you in reviewing this change, please?
>
> Yes! Is is merged in 3.9.x and master?
I have only opened it against 3.8.x so far. There is no JIRA raised as I tried to check if there is interest in such change at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] pzygielo commented on a diff in pull request #914: Generate or implement some `toString`s
Posted by GitBox <gi...@apache.org>.
pzygielo commented on code in PR #914:
URL: https://github.com/apache/maven/pull/914#discussion_r1050027356
##########
maven-model/src/test/java/org/apache/maven/model/OrganizationTest.java:
##########
@@ -53,4 +53,36 @@ public void testToStringNullSafe()
assertNotNull( new Organization().toString() );
}
+
+ public void testToStringNotNonsense11()
+ {
+ Organization org = new Organization();
+ org.setName( "Testing Maven Unit" );
+ org.setUrl( "https://maven.localdomain" );
+
+ assertEquals( "name = 'Testing Maven Unit'\nurl = 'https://maven.localdomain'", org.toString() );
Review Comment:
`MailingList`, `Scm`, `Organiation` - their `toString` is generated by modello.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] pzygielo commented on a diff in pull request #914: Generate or implement some `toString`s
Posted by GitBox <gi...@apache.org>.
pzygielo commented on code in PR #914:
URL: https://github.com/apache/maven/pull/914#discussion_r1050033376
##########
maven-model/src/test/java/org/apache/maven/model/BuildTest.java:
##########
@@ -53,4 +53,12 @@ public void testToStringNullSafe()
assertNotNull( new Build().toString() );
}
+ public void testToStringNotNonsense()
+ {
+ Build build = new Build();
+
+ String s = build.toString();
+
+ assert "Build(super=BuildBase(super=PluginConfiguration(super=PluginContainer())))".equals( s ) : s;
Review Comment:
> That's a very verbose and not really useful `toString()` ...
Also - I've stolen the format from https://projectlombok.org/features/ToString.
Once something more useful than hashCode is provided by super class - it will be included there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] pzygielo commented on a diff in pull request #914: Generate or implement some `toString`s
Posted by GitBox <gi...@apache.org>.
pzygielo commented on code in PR #914:
URL: https://github.com/apache/maven/pull/914#discussion_r1050028043
##########
maven-model/src/test/java/org/apache/maven/model/OrganizationTest.java:
##########
@@ -53,4 +53,36 @@ public void testToStringNullSafe()
assertNotNull( new Organization().toString() );
}
+
+ public void testToStringNotNonsense11()
+ {
+ Organization org = new Organization();
+ org.setName( "Testing Maven Unit" );
+ org.setUrl( "https://maven.localdomain" );
+
+ assertEquals( "name = 'Testing Maven Unit'\nurl = 'https://maven.localdomain'", org.toString() );
+ }
+
+ public void testToStringNotNonsense10()
+ {
+ Organization org = new Organization();
+ org.setName( "Testing Maven Unit" );
+
+ assertEquals( "name = 'Testing Maven Unit'\nurl = 'null'", org.toString() );
Review Comment:
I just let modello do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #914: Implement some `toString`s
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #914:
URL: https://github.com/apache/maven/pull/914#issuecomment-1356805024
> @michael-o - [[HEADS UP] Maven 3.8.7](https://lists.apache.org/thread/7p81o29z56k1kw1tc3k5j1t5fg4rsr2d) - may I interest you in reviewing this change, please?
Yes! Is is merged in 3.9.x and master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] pzygielo commented on pull request #914: Implement some `toString`s
Posted by GitBox <gi...@apache.org>.
pzygielo commented on PR #914:
URL: https://github.com/apache/maven/pull/914#issuecomment-1356799202
@michael-o - [[HEADS UP] Maven 3.8.7](https://lists.apache.org/thread/7p81o29z56k1kw1tc3k5j1t5fg4rsr2d) - may I interest you in reviewing this change, please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] pzygielo commented on a diff in pull request #914: Generate or implement some `toString`s
Posted by GitBox <gi...@apache.org>.
pzygielo commented on code in PR #914:
URL: https://github.com/apache/maven/pull/914#discussion_r1050028945
##########
maven-model/src/test/java/org/apache/maven/model/BuildTest.java:
##########
@@ -53,4 +53,12 @@ public void testToStringNullSafe()
assertNotNull( new Build().toString() );
}
+ public void testToStringNotNonsense()
+ {
+ Build build = new Build();
+
+ String s = build.toString();
+
+ assert "Build(super=BuildBase(super=PluginConfiguration(super=PluginContainer())))".equals( s ) : s;
Review Comment:
> The toString() result should at least have the simple class name in it.
Please suggest what else could be included, or what to leave.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #914: Generate or implement some `toString`s
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #914:
URL: https://github.com/apache/maven/pull/914#issuecomment-1353655731
@pzygielo the `toString()` methods generated by modello look very weird while the one that have been added manually in the modello schema looks ok.
If you really need to target 3.x, I'd suggest to fix the [Modello `generateToString` method](https://github.com/codehaus-plexus/modello/blob/master/modello-plugins/modello-plugin-java/src/main/java/org/codehaus/modello/plugin/java/JavaModelloGenerator.java#L410-L456) or to modify the [modello model](https://github.com/apache/maven/blob/maven-3.8.x/maven-model/src/main/mdo/maven.mdo) to manually add sensible `toString()` method similar to the [existing ones](https://github.com/apache/maven/blob/maven-3.8.x/maven-model/src/main/mdo/maven.mdo#L1132-L1135).
Or target the master branch where the generated classes can be more easily modified in the [velocity template](https://github.com/apache/maven/blob/master/api/maven-api-model/src/main/mdo/model.vm).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org