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

align lib-core #326

Merged
merged 33 commits into from
Aug 7, 2024
Merged

align lib-core #326

merged 33 commits into from
Aug 7, 2024

Conversation

gunplar
Copy link
Collaborator

@gunplar gunplar commented Aug 7, 2024

No description provided.

Alexander Lowey-Weber and others added 30 commits July 25, 2024 17:12
…onstructors are only functions, not to be used with new.
…n options, rename realm to map (sync with IMS).
…sh calculation, fix geo-grid calc, merge PgOptions with SessionOptions, plenty other fixes
# Conflicts:
#	here-naksha-lib-core/src/test/java/com/here/naksha/lib/core/JsonMappingTest.java
#	here-naksha-lib-model/src/commonMain/kotlin/naksha/model/FeatureEncoding.kt
# Conflicts:
#	here-naksha-lib-core/src/main/java/com/here/naksha/lib/core/models/storage/ContextResult.java
#	here-naksha-lib-core/src/main/java/com/here/naksha/lib/core/models/storage/ContextXyzFeatureResult.java
#	here-naksha-lib-core/src/main/java/com/here/naksha/lib/core/util/storage/ResultHelper.java
@gunplar gunplar requested a review from Amaneusz August 7, 2024 09:19
Copy link
Collaborator

@Amaneusz Amaneusz left a comment

Choose a reason for hiding this comment

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

Approved conditionally ;) discussed on webex

@@ -7,7 +7,7 @@ import java.time.format.DateTimeFormatter

repositories {
// provides dev versions!
maven("https://maven.pkg.jetbrains.space/kotlin/p/kotlin/bootstrap")
//maven("https://maven.pkg.jetbrains.space/kotlin/p/kotlin/bootstrap")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If not needed then maybe let's remove them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This may still serves as reminder for the guys later. I did not modify this part.

id("org.jetbrains.kotlin.multiplatform").version("2.1.0-dev-1329").apply(false)
kotlin("plugin.js-plain-objects").version("2.1.0-dev-1329")
//id("org.jetbrains.kotlin.multiplatform").version("2.0.20-Beta1").apply(false)
//kotlin("plugin.js-plain-objects").version("2.0.20-Beta1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If not needed then maybe let's remove them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are deleted lines

import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

class ActivityLogRequestTranslationUtil {

private static final String[] ACTIVITY_LOG_ID_PATH = new String[] {PROPERTIES, XYZ_ACTIVITY_LOG_NS, ID};
static final PRef PREF_ACTIVITY_LOG_ID = pRefFromPropPath(ACTIVITY_LOG_ID_PATH);
static final Property PREF_ACTIVITY_LOG_ID = new Property(ACTIVITY_LOG_ID_PATH);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are sure that these two lines are logically equal? This not a rhetorical question :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to be a result of refactoring by Alex, should be correct

// final PRef pref = RequestHelper.pRefFromPropPath(new String[]{"properties","prop_1"});
// assertNotNull(pref);
// assertTrue(pref instanceof NON_INDEXED_PREF, "Must be instanceof NonIndexedPRef");
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is shaky, but I understand that this might be not yours - let's add keast add a TODO comment to see if this test can get uncommented / deleted

//
// fun partition(): Boolean = true
//}
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add TODO comment to address it (delete test / repair it)

//import naksha.model.response.ErrorResponse
//import naksha.model.response.Response
//import naksha.model.response.SuccessResponse
//import naksha.model.request.condition.BufferTransformation.Companion.bufferInMeters
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add TODO comment to address this file (delete test / repair it)

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually I think you can mention all files in this package

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lib-psql is under Alex's and Pawel's decision, but I think they already marked these with TODOs, this file is marked line 615

// // then
// String expectedJson = "{\"collectionIds\":[],\"limit\":100000,\"limitVersions\":1,\"noFeature\":false,\"noGeoRef\":false,\"noGeometry\":false,\"noMeta\":false,\"noTags\":false,\"queryDeleted\":false,\"queryHistory\":false,\"resultFilter\":[],\"returnHandle\":false}";
// assertEquals(expectedJson, json, "there is a property change in ReadFeatures, add it to shallowCopy and update json");
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it have to be commented?
If you know how - try to fix this plz, otherwise add a TODO comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add a prefix, the TODO is already on line 14

JbFeatureDecoder jbFeatureDecoder = new JbFeatureDecoder(new JbDictManager()).mapBytes(featureBytes, 0, featureBytes.length);
Map<String, Object> featureAsMap = (Map<String, Object>) new JbMapDecoder().mapReader(jbFeatureDecoder.getReader()).toIMap();
JbRecordDecoder jbRecordDecoder = new JbRecordDecoder(new JbDictManager()).mapBytes(featureBytes, 0, featureBytes.length);
Map<String, Object> featureAsMap = (Map<String, Object>) new JbMapDecoder().mapReader(jbRecordDecoder.getReader()).toIMap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would be need javabak file?
If you know we need it - leave it. If you know that we don't - delete. If you don't know - add a TODO comment plz :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer to keep all of these as reference. There might be some functionalities that have not been ported into our new Kotlin classes, hence in some other unmigrated modules these will be called and result in compiler errors. It's better to have these to check what were actually performed underneath.

@gunplar gunplar merged commit 7b30842 into v3 Aug 7, 2024
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants