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

adds a basic maven dependency updater hint. #5009

Merged
merged 2 commits into from
Dec 30, 2022
Merged

adds a basic maven dependency updater hint. #5009

merged 2 commits into from
Dec 30, 2022

Conversation

mbien
Copy link
Member

@mbien mbien commented Nov 26, 2022 •
edited
Loading

  • marks artifacts which have a newer version available
  • provides a fix to bump the version
  • supports maven properties
    update-properties
  • ignores non-numerical versions discovered ComparableVersion :)
    update-deps
    • only suggests qualifiers like -SNAPSHOT if the used version is also using a qualifier
    • ignores some non-standard version formats. Like timestamps, since they are rarely used anymore.
    • can be restricted to not suggest major version upgrades
      artifact-upgrade-options

I wasn't sure whether or not to enable this by default but I decided to enable it mostly because:

  • many new-project-poms are outdated, this mitigates the issue a little bit (this is also the original motivation for the hint)
  • poms don't have a lot of hints right now, so adding a little bit of noise probably doesn't hurt atm
  • users can turn it off / we can turn it off based on feedback later

Performance seems good. It checks large poms in about a second. Maven hints are not cancellable unfortunately.

@mbien mbien added the Maven [ci] enable "build tools" tests label Nov 26, 2022
@mbien mbien added this to the NB17 milestone Nov 26, 2022
@mbien mbien force-pushed the maven-update-artifact-hint branch from 4c0a8a5 to e03fc81 Compare December 5, 2022 18:17
@mbien mbien marked this pull request as ready for review December 5, 2022 19:08
@mbien mbien force-pushed the maven-update-artifact-hint branch from e03fc81 to d631225 Compare December 15, 2022 05:50
@mbien mbien force-pushed the maven-update-artifact-hint branch from d631225 to bf34dff Compare December 22, 2022 00:29
@mbien
Copy link
Member Author

mbien commented Dec 22, 2022

only rebased to latest master

matthiasblaesing
matthiasblaesing reviewed Dec 26, 2022
View reviewed changes
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Implementation looks sane. Two thoughts:

  1. (Out of scope for this changeset): Maven hints are missing "Show as warning on current line". With more hints the noise goes up and the hepfulness goes down. This is a candiate for to much noise. @work this would flag nearly all dependencies and thus I would deactivate it.
  2. Would it make sense to provide an update to latest and "latest in same major version"? I associate bumping major version with incompatible change or at least "no stability guarentee".

@mbien mbien force-pushed the maven-update-artifact-hint branch from bf34dff to 922fa4c Compare December 28, 2022 02:25
@mbien
Copy link
Member Author

mbien commented Dec 28, 2022 •
edited
Loading

Implementation looks sane. Two thoughts:

1. (Out of scope for this changeset): Maven hints are missing "Show as warning on current line". With more hints the noise goes up and the hepfulness goes down. This is a candiate for to much noise. @work this would flag nearly all dependencies and thus I would deactivate it.

yeah would be nice to have feature parity with the java hints where it makes sense. There is also currently no way to run maven hints as inspections although this is probably only rarely needed. Projects can define project specific hints but this is also only possible for java hints I believe (I don't see anything else in the combo box).

2. Would it make sense to provide an update to latest and "latest in same major version"? I associate bumping major version with incompatible change or at least "no stability guarentee".

Done. I added it as option in the hint settings. I actually thought about adding this too but dismissed it since I initially wanted to keep this really simple (since there are maven plugins and dependabot for this). But I believe a simple "don't suggest major version upgrades" probably covers 95% of all typical cases, so why not.

The initial motivation for this was because many project pom templates are outdated, this gives the user a quick upgrade path even on first use once the IDE downloaded the maven index. The release version hint (and other IDE features too) require certain compiler plugin versions. This hint can basically unlock other hints by bumping this version.

matthiasblaesing
matthiasblaesing approved these changes Dec 29, 2022
View reviewed changes
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks sane to me.

@mbien mbien force-pushed the maven-update-artifact-hint branch from ecf07b8 to b3406cf Compare December 29, 2022 22:17
@mbien
Copy link
Member Author

mbien commented Dec 29, 2022

awesome, squashed PR to:

  • first commit will be the maven option panel polling bugfix
  • second commit is the feature

this should hopefully reduce risks and allow to test things independently if something happens. (I try to avoid fixing bugs in feature commits)

@mbien mbien mentioned this pull request Dec 29, 2022
Closed
mbien added 2 commits December 30, 2022 01:29
@mbien
fixes several maven HintsPanel bugs.
c6ec236 
for option customizers:

 - this fixes various issues with cancel behavior caused
   by hints logic not polling the saved values on second open
   of the options panel
 - fixes apply button activation when options change

for the main panel hint activation/deactivation logic:

 - fixes a bug when it was not possible to enable a hint again
   when previously disabled in the same session without a NB restart
 - this was caused by the fact that the interal saved value storage
   of the Configuration object was never reset before reuse
@mbien
adds a basic maven dependency updater hint.
0fdc498 
 - marks artifacts which have a newer version available
 - provides a fix to bump the version
 - supports maven properties
 - can optionally ignore major version upgrades
 - gives it's best to filter/compare non-standard versions
@mbien mbien force-pushed the maven-update-artifact-hint branch from b3406cf to 0fdc498 Compare December 30, 2022 00:37
@mbien
Copy link
Member Author

mbien commented Dec 30, 2022

@matthiasblaesing sorry for changing the PR after approval. I fixed #5165 too. See first commit.

@mbien mbien linked an issue Dec 30, 2022 that may be closed by this pull request
Maven hints can't be enabled again after they were disabled #5165
Closed
@matthiasblaesing
Copy link
Contributor

Could reproduce the problem and found it fixed with the update. Thank you.

@mbien mbien merged commit bb12195 into apache:master Dec 30, 2022
@mbien mbien mentioned this pull request Jan 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiasblaesing matthiasblaesing matthiasblaesing approved these changes

@sdedic sdedic Awaiting requested review from sdedic

@ebarboni ebarboni Awaiting requested review from ebarboni

Assignees
No one assigned
Labels
Editor hints Maven [ci] enable "build tools" tests
Projects
None yet
Milestone
NB17
Development

Successfully merging this pull request may close these issues.

Maven hints can't be enabled again after they were disabled
3 participants
@mbien @matthiasblaesing @emaphis