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: Don't typealias Month #174

Closed
wants to merge 1 commit into from

Conversation

wkornewald
Copy link
Contributor

@wkornewald wkornewald commented Dec 29, 2021

This is just one example from the codebase. Why are so many APIs expect and why do we even try to use typealias at all? This makes it more difficult to define a nice API for Kotlin and it causes lots of code repetition. It's even impossible to use sealed interface/class with expect/actual because the multiplatform code lives in different modules (see #173, #177 and #175 for how this can be useful).

I think it would be much better to separate the expect APIs and make them internal and expose a nice and clear non-expect based public API (which can call the internal expect API inside).

What do you think?

@wkornewald wkornewald closed this Jan 12, 2022
@wkornewald
Copy link
Contributor Author

@dkhalanskyjb Closing this PR. Is it even worth discussing in a separate issue? This could be a general architectural discussion and the Month typealias is just one aspect of the way expect/actual is used.

@dkhalanskyjb
Copy link
Collaborator

There's an issue for it already: #96

@wkornewald
Copy link
Contributor Author

wkornewald commented Jan 12, 2022

Not quite. That only focuses on the Month typealias. My suggestion is to generally apply the rule: avoid expect + public because it leads to problems like sealed not working (maybe the only exception is expect public fun which might be always ok?). Try to use expect only with private and internal.

@dkhalanskyjb
Copy link
Collaborator

The library is fairly compact, so its internal architecture can be changed on a whim once we need it and does not matter much. What matters is the API that we provide to the programmers, and they typically shouldn't care whether something is expect or not.

actual typealias has actual issues on that front: #96 (comment). Do you see any potential issues for the programmers because of, say, LocalDate being expect?

@wkornewald
Copy link
Contributor Author

The library is fairly compact, so its internal architecture can be changed on a whim once we need it and does not matter much. What matters is the API that we provide to the programmers, and they typically shouldn't care whether something is expect or not.

actual typealias has actual issues on that front: #96 (comment). Do you see any potential issues for the programmers because of, say, LocalDate being expect?

I don't think there's any similarly fatal issue. The other issue is that this whole library can't use sealed which limits the API design in general and indirectly affects how comfortably programmers can work with this lib. At least one can't easily provide a FlexibleDate/FlexibleDateTime in this lib and I find that unfortunate because we keep reinventing the wheel.

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