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 2020/10/23 13:01:17 UTC
[GitHub] [maven] mthmulders opened a new pull request #388: Refactor ProjectsCollector.collectProjects
mthmulders opened a new pull request #388:
URL: https://github.com/apache/maven/pull/388
This method now returns its result instead of modifying one of its arguments.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [X] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure please ask on the developers list.
- [X] I have signed the ICLA, and my employer has signed the CCLA.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] asfgit closed pull request #388: Refactor ProjectsCollector.collectProjects
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #388:
URL: https://github.com/apache/maven/pull/388
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on pull request #388: Refactor ProjectsCollector.collectProjects
Posted by GitBox <gi...@apache.org>.
mthmulders commented on pull request #388:
URL: https://github.com/apache/maven/pull/388#issuecomment-715329957
> Can you tell what the purpose is behind this change?
I identified this opportunity for improvement when I was working on [MNG-6118](https://issues.apache.org/jira/browse/MNG-6118), but that was complex enough already so I decided to postpone this little improvement and refactor it separately.
I think methods that `return` their results are better than methods that expect an argument which they can fill with a result, as the latter typically leads to code like this:
```java
List<?> whatever = new ArrayList<>();
doSomething(whatever, other, arguments, that, are, needed);
```
which obscures the fact that the `whatever` will be modified by invoking `doSomething`. After this change, the same code would look like this:
```java
List<?> whatever = doSomething(other, arguments, that, are, needed);
```
This is shorter and more clear (intention-revealing) IMO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on pull request #388: Refactor ProjectsCollector.collectProjects
Posted by GitBox <gi...@apache.org>.
mthmulders commented on pull request #388:
URL: https://github.com/apache/maven/pull/388#issuecomment-715389858
1. This interface was merged into master only five days ago, I don't think anyone has built a different implementation for it just yet...
1. If I understood correctly, the `ProblemCollector` you are referring to is part of a chain. The collector we're talking about in this PR is *not* a part of chain. Would the same argument apply in that case, even at the expense (if I may say so) of having code that reveals its intent and is harder to interpret for the human brain?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on pull request #388: Refactor ProjectsCollector.collectProjects
Posted by GitBox <gi...@apache.org>.
mthmulders commented on pull request #388:
URL: https://github.com/apache/maven/pull/388#issuecomment-719275697
[Jenkins build](https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven/job/refactor-projectscollector-collectprojects-return-result/2/) is green.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #388: Refactor ProjectsCollector.collectProjects
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #388:
URL: https://github.com/apache/maven/pull/388#issuecomment-716113439
@mthmulders I join @rfscholte opinion. The current naming isn't optimal.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #388: Refactor ProjectsCollector.collectProjects
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #388:
URL: https://github.com/apache/maven/pull/388#issuecomment-715327843
Can you tell what the purpose is behind this change?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #388: Refactor ProjectsCollector.collectProjects
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #388:
URL: https://github.com/apache/maven/pull/388#issuecomment-715345021
There are two issues here:
1. You are changin a public interface. I don't know whether this is acceptable
2. As far as I know collect methods they are supposed to receive where to collect it, see the usage of `ProblemCollector` in Core.
I expect collectors to use used in a uniform fashion throughout the Core codebase.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org