-
Notifications
You must be signed in to change notification settings - Fork 605
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
Use JcrPackageDefinition for retrieving metadata #3227
Use JcrPackageDefinition for retrieving metadata #3227
Conversation
This is less error prone and faster than acting on the compressed underlying ZIP binary Always close JcrPackageManager (although it should now no longer be necessary) Some refactoring This closes #3225
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3227 +/- ##
========================================
Coverage 1.34% 1.34%
Complexity 44 44
========================================
Files 721 721
Lines 29537 29515 -22
Branches 3844 3837 -7
========================================
Hits 397 397
+ Misses 29124 29102 -22
Partials 16 16 ☔ View full report in Codecov by Sentry. |
&& otherPackageId.get().getName().equals(thisPackageId.get().getName()); | ||
private boolean isLatestInstalled(JcrPackageDefinition referencePkgDefinition, Stream<JcrPackage> installedPackages) throws RepositoryException { | ||
try { | ||
Optional<@Nullable JcrPackageDefinition> lastInstalledPckDefinitionOptional = installedPackages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that @Nullable
annotation seems a bit odd to me. Isn't Optional<>
just covering the nullable case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that was a leftover from the refactoring. Fixed now.
try { | ||
return p.getDefinition(); | ||
} catch (RepositoryException e) { | ||
throw new UncheckedRepositoryException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this exception contain enough metadata to identify the package where the exception happened? Or should we add an additional message to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now enrich the relevant exception with the package path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This is less error prone and faster than acting on the compressed underlying ZIP binary
Always close JcrPackageManager (although it should now no longer be necessary)
Some refactoring
This closes #3225