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

NCL-6600 Add API for using latest version #840

Merged
merged 3 commits into from
Jun 17, 2021
Merged

NCL-6600 Add API for using latest version #840

merged 3 commits into from
Jun 17, 2021

Conversation

rnc
Copy link
Contributor

@rnc rnc commented Jun 15, 2021

No description provided.

@codecov
Copy link

codecov bot commented Jun 15, 2021 •
edited
Loading

Codecov Report

Merging #840 (96bd694) into master (2c72c9d) will increase coverage by 0.25%.
The diff coverage is 92.00%.

❗ Current head 96bd694 differs from pull request most recent head a4a23aa. Consider uploading reports for the commit a4a23aa to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master     #840      +/-   ##
============================================
+ Coverage     83.17%   83.42%   +0.25%     
+ Complexity     1871     1863       -8     
============================================
  Files           115      115              
  Lines          6940     6896      -44     
  Branches       1209     1201       -8     
============================================
- Hits           5772     5753      -19     
+ Misses          686      663      -23     
+ Partials        482      480       -2
Impacted Files Coverage Δ
...aven/ext/common/json/DependencyAnalyserResult.java 57.14% <50.00%> (ø)
...rg/commonjava/maven/ext/common/util/JSONUtils.java 93.33% <75.00%> (-2.02%) ⬇️
.../commonjava/maven/ext/core/impl/RESTCollector.java 83.33% <95.45%> (+3.06%) ⬆️
...monjava/maven/ext/core/groovy/BaseScriptUtils.java 68.29% <100.00%> (ø)
...mmonjava/maven/ext/core/impl/RESTBOMCollector.java 87.27% <100.00%> (ø)
...ommonjava/maven/ext/io/rest/DefaultTranslator.java 91.66% <100.00%> (+10.69%) ⬆️
...mmonjava/maven/ext/integrationtest/ITestUtils.java 84.67% <0.00%> (-1.46%) ⬇️
...monjava/maven/ext/core/impl/PluginManipulator.java 71.92% <0.00%> (-1.32%) ⬇️
...a/maven/ext/io/resolver/MavenLocationExpander.java 71.68% <0.00%> (-0.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c72c9d...a4a23aa. Read the comment docs.

@rnc rnc requested a review from dwalluck June 15, 2021 13:51
@rnc rnc force-pushed the NCL6600 branch 2 times, most recently from 36065bd to ad2dfa5 Compare June 15, 2021 15:50
@rnc rnc force-pushed the NCL6600 branch 2 times, most recently from 0945d93 to 2da3396 Compare June 16, 2021 14:27
@rnc rnc marked this pull request as ready for review June 16, 2021 14:32
@rnc
Copy link
Contributor Author

rnc commented Jun 16, 2021

@dwalluck Did you have any last comments on this else I'll merge it tomorrow

@rnc rnc merged commit 6a0fc31 into release-engineering:master Jun 17, 2021
@rnc rnc deleted the NCL6600 branch June 17, 2021 08:49
dwalluck
dwalluck reviewed Jun 17, 2021
View reviewed changes
io/src/main/java/org/commonjava/maven/ext/io/rest/Translator.java Show resolved Hide resolved
io/src/test/java/org/commonjava/maven/ext/io/rest/handler/AddSuffixJettyHandler.java Show resolved Hide resolved
core/src/main/java/org/commonjava/maven/ext/core/impl/RESTCollector.java
Comment on lines +124 to +131
Map<ProjectVersionRef, String> pvResultResult = state.getVersionTranslator().lookupProjectVersions( restLookupProjectVersionParamList );
logger.info ("REST Client returned for project versions: {} ", pvResultResult);
Map<ProjectRef, Set<String>> versionStates = new HashMap<>();
pvResultResult.forEach( ( key, value ) -> {
Set<String> versions = versionStates.computeIfAbsent( key.asProjectRef(), k -> new HashSet<>() );
versions.add( value );
} );
vs.setRESTMetadata( versionStates );
Copy link
Member

Choose a reason for hiding this comment

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

I am trying to understand the difference in the contents of pvResultResult and versionStates. It seems that they have virtually identical keys (ProjectVersionRef vs. ProjectRef), but the value is String vs. Set<String>. Is the value of versionStates always a singleton because there's only ever one key so there's only ever one value added? I feel like there is something obfuscated about this code.

In any case, we may be able to simplify with something like vs.setRESTMetadata( Stream.of( pvResultResult ).collect( Collectors.toMap() ) ).

io/src/main/java/org/commonjava/maven/ext/io/rest/DefaultTranslator.java Show resolved Hide resolved
io/src/main/java/org/commonjava/maven/ext/io/rest/DefaultTranslator.java Show resolved Hide resolved
io/src/main/java/org/commonjava/maven/ext/io/rest/DefaultTranslator.java Show resolved Hide resolved
@rnc
Copy link
Contributor Author

rnc commented Jun 17, 2021

For future reference, I have entered #846 to resolve comments raised after merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwalluck dwalluck dwalluck left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rnc @dwalluck