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

[WIP] Kotlin support #77

Closed
wants to merge 6 commits into from
Closed

[WIP] Kotlin support #77

wants to merge 6 commits into from

Conversation

levi
Copy link
Contributor

@levi levi commented Sep 3, 2017

Very much WIP. Currently builds off the other Swift PR.

Still determining a design strategy for the models, likely will use a hybrid of data classes and the builder pattern. Open to suggestions.

@ghost
Copy link

ghost commented Sep 3, 2017

🚫 CI failed with log

@ghost
Copy link

ghost commented Sep 3, 2017

3 Warnings
⚠️ PR is classed as Work in Progress
⚠️ This is a big PR, please consider splitting it up to ease code review.
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@ghost
Copy link

ghost commented Sep 3, 2017

🚫 CI failed with log

@rahul-malik
Copy link
Collaborator

@levi sounds great. I imagined we would generate data classes and as far as builders this approach seemed interesting

http://kotlinlang.org/docs/reference/type-safe-builders.html

@maicki
Copy link
Contributor

maicki commented Sep 5, 2017

Would this support conversion between Map / JSON <-> Model out of the box like Codable on the Swift side?

@levi
Copy link
Contributor Author

levi commented Sep 5, 2017

@maicki as far as I undertand, Kotlin doesn't currently have a codable-like feature. I was planning on taking the coder agnostic approach that we already do with the Obj-c implementation.

@rahul-malik
Copy link
Collaborator

@levi let me know how i can help move this one along as there are more unknowns than the Swift implementation. I can bring in some Kotlin folks at Pinterest to review the proposed generated code / patterns

let outputs: [String] = []
return render(lines: [
renderCommentHeader(),
"import Foundation",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Foundation? In Kotlin?

@christinalee
Copy link

Levi let me know if you want to strategize Kotlin implementation. Would be happy to help, as I have an interest in having Kotlin supported as well :)

@maicki
Copy link
Contributor

maicki commented Sep 15, 2017

Would be good to know what is are the missing pieces to get it over the finish line cc @levi. If we could get Kotlin support into Plank, so we test it within Pinterest that would be huge.

@levi
Copy link
Contributor Author

levi commented Sep 18, 2017

@christinalee would appreciate some insight on a builder pattern API you'd find useful. The rest of the work is pretty straight forward to implement.

@rahul-malik
Copy link
Collaborator

@christinalee
Copy link

@rahul-malik given the presence of named constructor variables, default copy implementations in data classs, and the ability to use lambdas with receivers like apply I'm not sure the DSL is necessary here. Need a bit more insight into Plank's structure before making a Builder pattern suggestion though, so will sync with you offline and then get back to @levi

@maicki
Copy link
Contributor

maicki commented Sep 18, 2017

Should we start an issue like the one we did with flow: #56 were we maybe figure out together the right API / patterns we will use?

@christinalee
Copy link

christinalee commented Sep 22, 2017

Would be happy to @maicki. Rahul Bkase and I also spent a few hours discussing this. We went down several different paths but ended up looping back to the traditional static-esque builder pattern, that has a data class with a companion object that handles the building like this example:

    companion object {
        private const val UNINITIALIZED_STR = "" // for example purposes only

        class Builder {
            private var uid: String = UNINITIALIZED_STR
            private var imgUrl: URL = URL(UNINITIALIZED_STR)

            fun uid(uid: String): Builder {
                this.uid = uid
                return this
            }
            
            fun url(imgUrl: URL): Builder {
                this.imgUrl = imgUrl
                return this
            }
            
            fun build(): Pin {
                return Pin(uid, imgUrl)
            }
        }
    }
} 

We discussed numerous other options (including the one linked above), but when considering method counts, performance, and java interoperability, this seemed to give us the most utility with the fewest sacrifices. Because the object is a data class, Kotlin callers can use the built in copy to create new pins with certain fields changed while Java callers can route through the explicit builder pattern (they also, of course, have access to the copy method but since Java doesn't support default parameters it's a glorified constructor from Java call sites and therefore isn't super useful). Callers from both languages benefit from the built in hashCode and equals generation on the data class.

Anywho, happy to hear feedback and continue iterating on this idea, but based on our explorations, wanted to throw this out as a starting suggestion

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.

None yet

4 participants