-
Notifications
You must be signed in to change notification settings - Fork 37
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
Kotlinx collection immutable #574
Conversation
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.
The naming here is backward. We should focus on the parent type nature - Immutable. Let's rename all these from Persistent*
-> Immutable*
. Persistent collections are just an implementation of that, and not actually the focus here.
Similarly, we should be looking for the Immutable*
types in the factories, as the current implementation only supports Persistent*
subtypes.
Finally, let's add tests to cover both Immutable*
and Persistent*
types (the latter just to verify we're gracefully handling the concrete types).
alias(libs.plugins.mavenPublish) | ||
} | ||
|
||
tasks.named<KotlinCompile>("compileTestKotlin") { |
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.
There's a DSL accessor for this
tasks.named<KotlinCompile>("compileTestKotlin") { | |
compileTestKotlin { |
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.
@ZacSweers
After committing this change, the project doesn't run tests. (I commit new changes without this change)
What do you think if I revert the changes?
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.
sorry should be tasks.compileTestKotlin
. That said, it would be helpful if you elaborated on what you mean by "doesn't run tests".
...le/src/main/kotlin/dev/zacsweers/moshix/immutable/adapters/ImmutableCollectionJsonAdapter.kt
Outdated
Show resolved
Hide resolved
3149b54
to
4b33f48
Compare
Co-authored-by: Zac Sweers <pandanomic@gmail.com>
Co-authored-by: Zac Sweers <pandanomic@gmail.com>
4b33f48
to
7203d59
Compare
val objList: ImmutableList<SomeObject>, // or PersistentList<SomeObject>, | ||
val objCollection: ImmutableCollection<SomeObject>, // or PersistentCollection<SomeObject>, | ||
val objMap: ImmutableMap<String, SomeObject>, // or PersistentMap<String, SomeObject>, | ||
val objectsSet: ImmutableMap<SomeObject>, // or PersistentMap<SomeObject>, |
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.
Is this correct?
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 was trying to minimize the model while it contain all types too.
What's your suggestion?
@ZacSweers
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 am saying that ImmutableMap
has two type parameters, it would be a syntax error to write ImmutableMap<SomeObject>
(just one argument). Did you mean to write something else?
...le/src/main/kotlin/dev/zacsweers/moshix/immutable/adapters/ImmutableCollectionJsonAdapter.kt
Outdated
Show resolved
Hide resolved
* val objList: ImmutableList<SomeObject>, // or PersistentList<SomeObject>, | ||
* val objCollection: ImmutableCollection<SomeObject>, // or PersistentCollection<SomeObject>, | ||
* val objMap: ImmutableMap<String, SomeObject>, // or PersistentMap<String, SomeObject>, | ||
* val objectsSet: ImmutableMap<SomeObject>, // or PersistentMap<SomeObject>, |
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.
Same question about correctness here
} | ||
|
||
override fun toJson(writer: JsonWriter, value: E?) { | ||
writer.beginArray() |
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.
If the value is null, we should emit a nullValue and return, not an empty array. That said, we also know this will never be null due to the nullSafe() use so let's just force it to non-null
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 checked moshi and it uses markNotNull
but it's internal and I used a simple requireNotNull() for that. Is it suit?
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.
Neither, I am referring to JsonWriter.nullValue()
|
||
override fun toJson(writer: JsonWriter, map: PersistentMap<K, V?>?) { | ||
writer.beginObject() | ||
map?.forEach { (key, value) -> |
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.
Same here re: nullability
...le/src/main/kotlin/dev/zacsweers/moshix/immutable/adapters/ImmutableCollectionJsonAdapter.kt
Outdated
Show resolved
Hide resolved
Implemented in #586 |
Resolve issue #460 or 1739 of moshi
In following of #478, I implemented Collection, Set and did some improvements based on @ZacSweers comment: