Using kotlin data classes

[qnga]

I am writing unit tests for my new PackageDocumentParser and once again I have some issues with Shared module coding style because Publication content objects don't implement an equals method.
If that's ok for you, I suggest I refactor Publication model into safer and more idiomatic Kotlin (data classes with immutable members).

[mickael-menu]

I'm actually refactoring the Publication models right now, I guess it should be done by the beginning of next week but it probably won't be merged before some time.

I faced the same issue as you while adding unit tests for the JSON parsing, so I'm using data classes. However, I'd love to use immutable members but I fear that it would be too disruptive because a lot of places throughout R2 is expecting them to be mutable. I'm changing at least the inner mutable lists/maps to immutable types (they are still writable properties though) to prevent any bug if a data class is copied.

[mickael-menu]

Also feel free to import the following samples for your tests from the Swift version: https://github.com/readium/r2-streamer-swift/tree/develop/r2-streamer-swiftTests
and you can find the associated test cases here: https://github.com/readium/r2-streamer-swift/tree/develop/r2-streamer-swiftTests/Parser

Btw, maybe it makes sense that you update your branch with the XML parser by merging the model refactoring before it is merged into develop, this way you will be able to move forward. If you use only the constructors to build the models it will probably make it much easier to make the models immutables if we decide to do so later on. I'll keep you posted for the refactoring.

[qnga]

Okay, I adapt my Epub Parser to the new Publication model as soon as you've finished it. That'll much better like that. By the way, I suggest you group all classes used in Publication into a Publication package.

Concerning testing, I have started from Swift tests indeed, thanks.

[mickael-menu]

I'll be following the folder structure from Swift for the packages, to stay consistent – so yes, with a publication root package: https://github.com/readium/r2-shared-swift/tree/develop/r2-shared-swift/Publication

However, I still need to figure out how to move the types to other packages while staying backward compatible for the imports in third-party apps. Maybe a typealias would work, do you have some experience with this?

By the way, since we're creating the first unit tests in R2 Kotlin, I'm following some leads from this page for the best practices: https://phauer.com/2018/best-practices-unit-testing-kotlin/

In particular, I like the function names with the backticks which makes them much more readable in the Tests panel.

@Test
fun `parse JSON {rel} as single string`() {
    assertEquals(
        Link(href = "a", rels = listOf("publication")),
        Link.fromJSON(JSONObject("{'href': 'a', 'rel': 'publication'}"))
    )
}

[qnga]

God, are there really third-party apps that heavily depend on the publication model? I thought you were going to break the navigator API, why not to break the shared API too?
I guess typealiases in a Compatibility.kt file declaring package org.readium.r2.shared should be fine. But switching to data classes, you will necessarily break Publication construction. Maybe that's not the part used by third-party apps....

Thanks for the link about testing. Should we move to JUnit 5 right now?

[mickael-menu]

Theoretically, we're the only ones creating the shared model objects, but the apps are importing them for sure. All the code in the test app is supposed to be owned by third-party apps.

For the Navigator API, as much as possible the changes will be non-breaking, with deprecated functions that will be aliased to the new API.

So far I stayed on JUnit 4, I didn't need the new features. But feel free to upgrade if you need.

[qnga]

Fixed by refactoring of Publication model in PR readium/r2-shared-kotlin#88