You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2020/07/10 15:03:13 UTC
[GitHub] [sling-org-apache-sling-feature-extension-apiregions] bosschaert opened a new pull request #8: SLING-9521 Packages exported in earlier API Regions are not available to later API Regions
bosschaert opened a new pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/8
To support this the launcher extension now puts a property in the
features.properties map that contains the total ordering of regions,
e.g.:
__region.order__=global,deprecated,internal
This commit also fixes the fact that the region values in the
features.properties map were unordered. Features have an order and this
is now preserved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-extension-apiregions] cziegeler commented on a change in pull request #8: SLING-9521 Packages exported in earlier API Regions are not available to later API Regions
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/8#discussion_r454458107
##########
File path: src/main/java/org/apache/sling/feature/extension/apiregions/launcher/LauncherProperties.java
##########
@@ -101,18 +105,22 @@ public static Properties getBundleIDtoFeaturesMap(Feature app) {
}
public static Properties getFeatureIDtoRegionsMap(ApiRegions regions) {
- Map<ArtifactId, Set<String>> map = new HashMap<>();
+ Map<ArtifactId, List<String>> map = new HashMap<>();
for (ApiRegion region : regions.listRegions())
{
for (ArtifactId featureId : region.getFeatureOrigins()) {
map.compute(featureId, (id, regionNames) -> {
if (regionNames == null) {
- regionNames = new HashSet<>();
+ regionNames = new LinkedList<>();
}
regionNames.add(region.getName());
+ int insertionIndex = regionNames.size() - 1;
for (ApiRegion parent = region.getParent(); parent != null; parent = parent.getParent()) {
- regionNames.add(parent.getName());
+ String parentName = parent.getName();
+ if (!regionNames.contains(parentName)) {
Review comment:
I didn't realize the outer loop, so yes, makes sense
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-extension-apiregions] bosschaert commented on a change in pull request #8: SLING-9521 Packages exported in earlier API Regions are not available to later API Regions
Posted by GitBox <gi...@apache.org>.
bosschaert commented on a change in pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/8#discussion_r454364120
##########
File path: src/main/java/org/apache/sling/feature/extension/apiregions/launcher/LauncherProperties.java
##########
@@ -101,18 +105,22 @@ public static Properties getBundleIDtoFeaturesMap(Feature app) {
}
public static Properties getFeatureIDtoRegionsMap(ApiRegions regions) {
- Map<ArtifactId, Set<String>> map = new HashMap<>();
+ Map<ArtifactId, List<String>> map = new HashMap<>();
for (ApiRegion region : regions.listRegions())
{
for (ArtifactId featureId : region.getFeatureOrigins()) {
map.compute(featureId, (id, regionNames) -> {
if (regionNames == null) {
- regionNames = new HashSet<>();
+ regionNames = new LinkedList<>();
}
regionNames.add(region.getName());
+ int insertionIndex = regionNames.size() - 1;
for (ApiRegion parent = region.getParent(); parent != null; parent = parent.getParent()) {
- regionNames.add(parent.getName());
+ String parentName = parent.getName();
+ if (!regionNames.contains(parentName)) {
Review comment:
Hi @cziegeler this code iterates over multiple API Regions, which means that without the check the values on the `map` can have duplicates. It's easy to see that by removing the check, it will cause 2 test failures.
Previously this was not needed because the values on the map were of type `Set`...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-extension-apiregions] cziegeler commented on pull request #8: SLING-9521 Packages exported in earlier API Regions are not available to later API Regions
Posted by GitBox <gi...@apache.org>.
cziegeler commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/8#issuecomment-657489391
Lgtm, with the single suggested change
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-extension-apiregions] bosschaert commented on pull request #8: SLING-9521 Packages exported in earlier API Regions are not available to later API Regions
Posted by GitBox <gi...@apache.org>.
bosschaert commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/8#issuecomment-658240289
I went ahead and merged it. As explained I think the check is needed (as is exposed by the test failures if I take it out). @cziegeler if you think we should handle it a different way I would be happy to in a separate PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-extension-apiregions] bosschaert merged pull request #8: SLING-9521 Packages exported in earlier API Regions are not available to later API Regions
Posted by GitBox <gi...@apache.org>.
bosschaert merged pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/8
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-extension-apiregions] cziegeler commented on a change in pull request #8: SLING-9521 Packages exported in earlier API Regions are not available to later API Regions
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/8#discussion_r453566560
##########
File path: src/main/java/org/apache/sling/feature/extension/apiregions/launcher/LauncherProperties.java
##########
@@ -101,18 +105,22 @@ public static Properties getBundleIDtoFeaturesMap(Feature app) {
}
public static Properties getFeatureIDtoRegionsMap(ApiRegions regions) {
- Map<ArtifactId, Set<String>> map = new HashMap<>();
+ Map<ArtifactId, List<String>> map = new HashMap<>();
for (ApiRegion region : regions.listRegions())
{
for (ArtifactId featureId : region.getFeatureOrigins()) {
map.compute(featureId, (id, regionNames) -> {
if (regionNames == null) {
- regionNames = new HashSet<>();
+ regionNames = new LinkedList<>();
}
regionNames.add(region.getName());
+ int insertionIndex = regionNames.size() - 1;
for (ApiRegion parent = region.getParent(); parent != null; parent = parent.getParent()) {
- regionNames.add(parent.getName());
+ String parentName = parent.getName();
+ if (!regionNames.contains(parentName)) {
Review comment:
This check is not needed - the ApiRegions api ensures that the name of a region is unique
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-feature-extension-apiregions] sonarcloud[bot] commented on pull request #8: SLING-9521 Packages exported in earlier API Regions are not available to later API Regions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-feature-extension-apiregions/pull/8#issuecomment-656723415
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100.png' alt='100.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature-extension-apiregions&pullRequest=8&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org