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

Caching, Last Version on XML reports, Remove Oldest #732

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

sultan
Copy link
Contributor

@sultan sultan commented Oct 2, 2022

Edit: refactored the PR following Andrzej job, added caching for calls on "Newest" methods, removed "Oldest" methods
Still open to feedback before we merge this,

i would like to suggest:

  • removal of all the NEXT labels in the HTML reports, only the LATEST labels are usefull.
  • removal of all the OLDEST methods and replace calls by NEWEST
    this suggestion impacts the XML reports (tag lastVersion instead of nextVersion)

this should normally not impact the "useNext" Mojos

@sultan sultan force-pushed the Caching branch 2 times, most recently from 7290571 to 0af2eb9 Compare October 2, 2022 17:05
@andrzejj0
Copy link
Contributor

Just a heads up ;)

I'm preparing a chunky PR unifying those renderers. I've introduced a common model and managed to largely unify them. Work not done since there's still some area of improvement.

master...ajarmoniuk:versions-maven-plugin:doxia-renderers

@sultan
Copy link
Contributor Author

sultan commented Oct 2, 2022

ok i'll put my work on halt until then, my needs would be being able to access your model in any report method.
that would help me filter out things for my other pr

@andrzejj0
Copy link
Contributor

I think this PR is made redundant by #749

@sultan
Copy link
Contributor Author

sultan commented Oct 15, 2022

I think this PR is made redundant by #749

That's hopefully ok, let me rework my PR and see if i need any additional refactoring.

We chatted about some reworks, there : #454 (comment)

my suggestions seems still relevant, i am eager to do the job, but i'd like feedback on the idea from you @ajarmoniuk or anybody :)

about improvements on -DallowAnyUpdates=false -DallowMajorUpdates=false :
allowMinor allowMinor etc could be replaced by an enum with NULL/EMPTY equivalent to MAJOR,
examples
-DupdateScope=major
-DupdateUpTo=major

Subinc Inc Minor Major
YES YES YES YES Major/Any/All/Empty/Null/
YES YES YES no Minor/MinorOrLess
YES YES no no Inc/IncOrLess
YES no no no SubInc/SubincOnly

others are a bit more complicated, i would like to introduce the possibility to disallow previews :

Snapshots Previews :
Milestones RCs
Releases
YES YES YES Any/All/Empty/Null
YES no no Snapshots Only
no YES no Previews Only
no no YES Releases Only
no YES YES All but Snapshots
YES no YES All but Previews
YES YES no All but Releases

@andrzejj0
Copy link
Contributor

I think this PR is made redundant by #749

That's hopefully ok, let me rework my PR and see if i need any additional refactoring.

We chatted about some reworks, there : #454 (comment)

my suggestions seems still relevant, i am eager to do the job, but i'd like feedback on the idea from you @ajarmoniuk or anybody :)

about improvements on -DallowAnyUpdates=false -DallowMajorUpdates=false : allowMinor allowMinor etc could be replaced by an enum with NULL/EMPTY equivalent to MAJOR, examples -DupdateScope=major -DupdateUpTo=major
Subinc Inc Minor Major
YES YES YES YES Major/Any/All/Empty/Null/
YES YES YES no Minor/MinorOrLess
YES YES no no Inc/IncOrLess
YES no no no SubInc/SubincOnly

others are a bit more complicated, i would like to introduce the possibility to disallow previews :
Snapshots Previews :
Milestones RCs Releases
YES YES YES Any/All/Empty/Null
YES no no Snapshots Only
no YES no Previews Only
no no YES Releases Only
no YES YES All but Snapshots
YES no YES All but Previews
YES YES no All but Releases

This is also largely handled. Since the interface should not have been changed (otherwise this would constitute a big change to the users and would probably cause a lot of new bug reports), I have simply implemented a change request that the allowMinorUpdates being false should imply allowMajorUpdates false. Which also implies allowIncrementalUpdates false implying both allowMajorUpdates and allowMinorUpdates equal to false.

Internally we represent this as Optional<Segment> across the board. Which is basically the enum we wanted, with all updates represented as empty(). Arguable whether this shouldn't be made a separate value of the Segment, in my opinion it's not.

@sultan
Copy link
Contributor Author

sultan commented Oct 15, 2022

okay thanks for the quick reply,

let's not change theses flags for the users then.

back on the caching, i think i need some work on caching results on the property report

i'll try something

@sultan sultan marked this pull request as ready for review October 15, 2022 14:33
@sultan
Copy link
Contributor Author

sultan commented Oct 15, 2022

Still open to feedback before we merge this,

i would like to suggest the removal of all the NEXT labels in the HTML reports
only the LATEST labels are usefull.

i would like to suggest the removal of all the OLDEST methods and replace calls by NEWEST
this suggestion impacts the XML reports (tag lastVersion instead of nextVersion)

this should normally not impact the "useNext" Mojos

@sultan sultan changed the title caching repeated call of methods caching repeated call of getNewest methods Oct 15, 2022
@sultan sultan changed the title caching repeated call of getNewest methods caching repeated call of getNewestXXX methods Oct 15, 2022
@sultan sultan force-pushed the Caching branch 3 times, most recently from 2e1b614 to cdf429e Compare October 16, 2022 12:37
@sultan sultan changed the title caching repeated call of getNewestXXX methods Caching, Last Version on XML reports, Remove Oldest Oct 16, 2022
@sultan sultan marked this pull request as draft October 16, 2022 13:07
@sultan sultan marked this pull request as ready for review October 16, 2022 14:04
@slawekjaranowski slawekjaranowski added this to the 2.13.0 milestone Oct 16, 2022
@sultan
Copy link
Contributor Author

sultan commented Oct 17, 2022

Thanks @slawekjaranowski

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

Successfully merging this pull request may close these issues.

3 participants