search

Electric-Coin-Company / kotlin-bip39

A concise implementation of BIP-0039 in Kotlin for Android. In order, it prioritizes being secure, concise and idiomatic.
MIT License
35 stars 9 forks source link

Separate Common and JVM-only code #158

Closed luca992 closed 1 year ago

luca992 commented 1 year ago

Should we make the new classes in the common and crypto packages internal? I think they're implementation details and probably shouldn't be a public API. I recognize that previously FallbackProvider and Pbkdf2Sha512 were public, and I suspect that was by accident when initially implemented. Making those two internal classes would technically be a public API change. Should we change them now? Should we keep them all public for now, then file an issue to make them all consistently internal as a followup?

I fixed the formatting issues. The probably should be internal. But up to you, I can make them all internal if you want

ccjernigan commented 1 year ago

Should we make the new classes in the common and crypto packages internal? I think they're implementation details and probably shouldn't be a public API. I recognize that previously FallbackProvider and Pbkdf2Sha512 were public, and I suspect that was by accident when initially implemented. Making those two internal classes would technically be a public API change. Should we change them now? Should we keep them all public for now, then file an issue to make them all consistently internal as a followup?

I fixed the formatting issues. The probably should be internal. But up to you, I can make them all internal if you want

Let's go ahead and make them all internal (including the two pre-existing classes of FallbackProvider and Pbkdf2Sha512 that should probably have been internal all along), which I think is a good improvement. After that, I think this will be good to go! The CI job already passed, so that's looking good too.

luca992 commented 1 year ago

@ccjernigan all made internal. It wasn't possible to make an actual internal typealias so I changed it to internal actual interface Closeable : java.io.Closeable

ccjernigan commented 1 year ago

@ccjernigan all made internal. It wasn't possible to make an actual internal typealias so I changed it to internal actual interface Closeable : java.io.Closeable

Great, thank you for figuring out a good option for this!