Conversation
Signed-off-by: Jakub Amanowicz <jakub.amanowicz@here.com>
Signed-off-by: Jakub Amanowicz <jakub.amanowicz@here.com>
Marcin-Here
left a comment
Marcin-Here
left a comment
There was a problem hiding this comment.
🥇 good job! few minor comments.
here-naksha-lib-mom10/README.md
Outdated
|
|
||
| #### Populating old delta | ||
|
|
||
| | `@ns:com:here:mom:delta` properties supported in Naksha V2 (pre MOM 10) | equivalents in `meta.moderationInfo` (MOM 10+) | |
here-naksha-lib-mom10/README.md
Outdated
| - we return feature in the same shape as it was given to Naksha in the writing phase | ||
|
|
||
| The class responsible for these operations | ||
| is [Mom10Transformation](../java/com/here/naksha/mom10/transform/Mom10Transformation.java). |
There was a problem hiding this comment.
I think "src/jvmMain/java/com/here/naksha/mom10/Mom10Transformation.java" this would be working path for Mom10Transformation
| } | ||
|
|
||
| public static void dropPreMom10Namespaces(@Nullable NakshaFeature feature) { | ||
| NakshaProperties properties = feature.getProperties(); |
There was a problem hiding this comment.
If feature is Nullable, shouldn't we guard for that before calling .getProperties()?
There was a problem hiding this comment.
We definitely should! Fixed :)
|
|
||
| public class Mom10Verification { | ||
|
|
||
| private static final Pattern SHORT_SEM_VER = Pattern.compile("^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\..+$"); |
| } | ||
|
|
||
| private static @Nullable Integer getMajorFrom(@NotNull String modelVersion) { | ||
| Matcher matcher = SHORT_SEM_VER.matcher(modelVersion); |
There was a problem hiding this comment.
In tests we have case as ""8.91" so if this is possible version, then let's say "10.0" is also possible and if so then our regex "\..+$" would return false on it. Is this scenario possible?
There was a problem hiding this comment.
It is possible and we treat "10.0" as invalid value, "10.0.0" is ok though
There was a problem hiding this comment.
There was a mistake in tests though, fixed
| } | ||
|
|
||
| private static Stream<Named<VerificationCase>> shouldVerifyIfFeatureIsInMom10() { | ||
| return Stream.of( |
There was a problem hiding this comment.
if above comment about "10.0" scenario is possible, let's add tests for it. If no, ignore this comment.
Signed-off-by: Jakub Amanowicz <jakub.amanowicz@here.com>
* cherry picked mom10 from v2 Signed-off-by: Jakub Amanowicz <jakub.amanowicz@here.com> * CASL-1466 migrated mom10 module to v3 Signed-off-by: Jakub Amanowicz <jakub.amanowicz@here.com> * CASL-1466 review fixes Signed-off-by: Jakub Amanowicz <jakub.amanowicz@here.com> --------- Signed-off-by: Jakub Amanowicz <jakub.amanowicz@here.com>
2 participants
No description provided.