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.

Sign up for GitHub

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

Jump to bottom

#614 resolve version from model properties if necessary #797

Merged
merged 1 commit into from
Oct 28, 2022
Merged

#614 resolve version from model properties if necessary #797

merged 1 commit into from
Oct 28, 2022

Conversation

TorstenKruse
Copy link
Contributor

new branch name

@TorstenKruse TorstenKruse mentioned this pull request Oct 26, 2022
Closed
@andrzejj0
Copy link
Contributor

Please squash commits when done.

@andrzejj0
Copy link
Contributor

@slachiewicz Could you please approve the workflow? Thanks.

@TorstenKruse
Copy link
Contributor Author

Sorry for asking, but how to squash existing commits? Isn't this done during merge into master?

Another question, no hurry, just for my further planing, when can I expect the next release with this fix?

@andrzejj0
Copy link
Contributor

You can force push the commits to origin.

@andrzejj0
Copy link
Contributor

I mean, first squash the commits in Git:

Screen Shot 2022-10-26 at 17 25 31

Then, select the drop down and pick "Force Push":

Screen Shot 2022-10-26 at 17 25 59

@andrzejj0
Copy link
Contributor

Another question, no hurry, just for my further planing, when can I expect the next release with this fix?

A question to @slawekjaranowski probably ;)

@slachiewicz slachiewicz changed the title issue-614 resolve version from model properties if necessary #614 resolve version from model properties if necessary Oct 26, 2022
@slawekjaranowski slawekjaranowski linked an issue Oct 26, 2022 that may be closed by this pull request
display-dependency-updates does not resolve versions specified via properties if processDependencyManagementTransitive=false #614
Closed
@slawekjaranowski
Copy link
Member

@TorstenKruse thanks for PR.

In mentioned issue we have that display-dependency-updates are also affected.
As I see you fix in dependency-updates-report - can you also check display-dependency-updates?

@slawekjaranowski
Copy link
Member

Another question, no hurry, just for my further planing, when can I expect the next release with this fix?

A question to @slawekjaranowski probably ;)

Lat release was at 23 Oct,

As I see how many changes we have and how many contributors I'm very happy ...
So to have valuable feedback I have plan to release about once a month or two.

@TorstenKruse
Copy link
Contributor Author

Fixed for display-dependency-updates too.
I'm still not sure, if my version interpolation is sitting at the right place, perhaps someone with deeper knowledge of the project checks again.

@slawekjaranowski
Copy link
Member

@TorstenKruse please also rebase with current codebase and resolve conflicts.

@slawekjaranowski slawekjaranowski added this to the 2.14.0 milestone Oct 27, 2022
@TorstenKruse TorstenKruse force-pushed the issue-614 branch 2 times, most recently from 735592b to cf83b76 Compare October 28, 2022 08:34
@TorstenKruse
Copy link
Contributor Author

@slawekjaranowski Done. Hoping for a quick merge before more codebase changes. ;)

slawekjaranowski
slawekjaranowski reviewed Oct 28, 2022
View reviewed changes
src/main/java/org/codehaus/mojo/versions/api/DefaultVersionsHelper.java Outdated
return dependency;
}

private List<String> getSplitProperties( String commaSeparatedProperties )
Copy link
Member

Choose a reason for hiding this comment

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

Where this new private method is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not from me. Perhaps came from the rebase? Should I remove it?

Copy link
Contributor

@andrzejj0 andrzejj0 Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please don't remove it. It's being used in AbstractVersionsHelper. I changed the name from "splitted", that's why it appeared here. However, in my own PR, I've made some refactoring so that the method's gone.

EDIT: I can't find the method either. Please check if it's really there in master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem so. I removed it now.

@andrzejj0 andrzejj0 mentioned this pull request Oct 28, 2022
Merged
@slawekjaranowski slawekjaranowski merged commit 6647b8d into mojohaus:master Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slawekjaranowski slawekjaranowski slawekjaranowski left review comments

@andrzejj0 andrzejj0 andrzejj0 approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
2.14.0
3 participants
@TorstenKruse @andrzejj0 @slawekjaranowski