Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reload Project and recursively all Modules #324

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Reload Project and recursively all Modules #324

merged 1 commit into from
Jan 19, 2022

Conversation

dirk-kaspar
Copy link

The Changes suggested from @beatngu13 in Issue #130

@dirk-kaspar dirk-kaspar marked this pull request as ready for review January 14, 2022 15:25
@dirk-kaspar
Copy link
Author

Thats the first time I've used BeanShell. So please tell me if I have done something weird or strange 😃

@aleksandr-m
Copy link
Owner

The test seems to hold too much, probably some simple goal would be enough, but it is not a big deal.

Can you think of any negative impact of building all the projects can lead to?

Please squash all changes to single commit and force push to this branch.

@dirk-kaspar
Copy link
Author

I would love to see a simpler, more elegant solution! But that was the naive solution to demonstrate our Problem.
Building all the projects in this case is reloading all the pom and build the Model; Not an actual build for those modules or am I wrong?

All commits squashed.


for (ProjectBuildingResult projectBuildingResult : result) {
MavenProject resultProject = projectBuildingResult.getProject();
if (!resultProject.isExecutionRoot()) continue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to use continue instead of positive if and return? Also can you use brackets {} even for one line if.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With JDK 8 onward I would write it like

    private MavenProject reloadProject(final MavenProject project) throws MojoFailureException {
        try {
            return projectBuilder
                    .build(
                            Collections.singletonList(project.getFile()),
                            true,
                            mavenSession.getProjectBuildingRequest())
                    .stream()
                    .map(ProjectBuildingResult::getProject)
                    .filter(MavenProject::isExecutionRoot)
                    .findAny().orElseThrow(() -> new NoSuchElementException(
                            "None of the  ProjectBuildingResults is for the executionRoot"));
        } catch (Exception e) {
            throw new MojoFailureException("Error re-loading project info", e);
        }
    }

The notation with continue seems closer to the stream filter notation to me. But I don't mind changing it to a positive if condition.

@aleksandr-m
Copy link
Owner

Well, let's see how it behaves in real life then :)

</dependency>
</dependencies>
</dependencyManagement>
</project>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Some POMs end with a newline (like this), others don't (like src/it/release-start-finish-2-it/expectations/develop/withAgregatorAsParent/expected-pom.xml).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also some pom.xml files hat an indentation with of only two spaces. Fixed that too.

@beatngu13
Copy link
Contributor

The test seems to hold too much, probably some simple goal would be enough, but it is not a big deal.

This is my gut feeling, too. @dirk-kaspar let's have a look afterwards, maybe we can open another PR improving (i.e. minimizing) the IT.

Can you think of any negative impact of building all the projects can lead to?

Performance, maybe? We didn't do a proper performance test, but since we do a complete rebuild, this might increase execution time. On the other hand: we didn't observe anything becoming slower, at least in our pipelines.

Co-authored-by: beatngu13 <daniel.kraus@mailbox.org>
@aleksandr-m aleksandr-m merged commit e30d065 into aleksandr-m:master Jan 19, 2022
@aleksandr-m
Copy link
Owner

Thank you. Good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants