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

Hacktoberfest! #1

Open
NotJustAnna opened this issue Oct 4, 2022 · 4 comments
Open

Hacktoberfest! #1

NotJustAnna opened this issue Oct 4, 2022 · 4 comments

Comments

@NotJustAnna
Copy link
Member

NotJustAnna commented Oct 4, 2022

Hacktoberfest!

Hey there! If you're reading this, you're probably here for Hacktoberfest. If you're not, you're probably here because you're interested in nanoflakes. Either way, welcome!

Note

This is a child issue to nanoflakes/.github#4, the main Hacktoberfest issue for Nanoflakes. Use this issue to discuss improvements and contributions for the Kotlin implementation of Nanoflakes.

What's a Nanoflake?

Nanoflakes is a specification for unique identifiers which can be generated locally or from a remote server. The specification is designed to be simple and easy to implement, and to be compatible with Twitter's Snowflake algorithm, which Nanoflakes is based on.

The official, full specification can be found here.

I want to contribute!

Awesome! You can contribute to the Kotlin implementation by submitting PRs to this repository!

You can improve on the Kotlin implementation by...

  • Fixing bugs
  • Improving the code's documentation
  • Suggesting improvements to the codebase

If you have any other ideas for how you can contribute, please open an issue or
pull request to suggest them in this issue!

End note

If you have any questions, feel free to ask them in the comments below!

Nanoflakes is my hobby project, and I'm doing this for fun. I'm not expecting
anyone to contribute, but if you do, I'll be happy to accept your pull requests!

@lost-illusi0n
Copy link

Hey Adrian,
Found this through Hacktoberfest and I think this is a great project. It being compatible with Twitter snowflakes and having Kotlin MPP support are big ups, and I would like to keep it updated with the Kotlin ecosystem.

Improvements

Let me know what you think about the following:

  • adding support for additional Kotlin targets (linux and windows). DateTime may not be as straight-forward. Maybe kotlinx-datetime would be useful here. Or see my second point in the design section.

  • adding componentN functions so we can destructure nanoflakes. Would destructure into the 3 fields of a nanoflake.

  • there are lot of getter like methods in Nanoflake. Idiomatic Kotlin would rather these simply be vals imo.

  • making Nanoflake a data class around value and epoch, this would remove a bit of boilerplate in that class.

Design

The following are more of a larger design proposal which could warrant its own issue or flat out ignored. These come out of a feeling as if I'm using a Java library instead of one made for Kotlin.

  • Get rid of baseEpoch being an integral part to the Nanoflake structure. AFAIK it is not part of the Nanoflake structure according to the spec and it seems to just be a convenience thing. Maybe something like a NanoflakeWithEpoch or similarly named, would be appropriate here instead. It would wrap a Nanoflake, take an additional baseEpoch, and add the additional functionality involving the baseEpoch. Though a different alternative could be great as well.

  • Getting rid of baseEpoch would allow for Nanoflake becoming a value class around the value. This should remove the overhead of creating a new instance for every Nanoflake as it'll eventually be erased into a long. We can still keep destructuring, and utility functions. This one seems like a big improvement.

  • Would it make sense to make generators suspending? Currently, when generating a nanoflake, if the clock moves backwards, it will throw an exception for x ms. Instead, we could also provide a suspending API which will simply delay for that long until our timestamp is going forward again. I'm not crazy for this since it could be considered out of scope but might be good to think about.


Let me know how you feel about these and what warrant their own issues to further discuss them 😄.

@NotJustAnna
Copy link
Member Author

Hey @lost-illusi0n! Thank you for showing interest regarding the project!

adding support for additional Kotlin targets (linux and windows).

I totally agree! At the time that I implemented the Kotlin library, I didn't try to dip my toes with Kotlin/Native since it was pretty experimental, but having used it for a bit I think that we can add more Kotlin/Native targets.

adding componentN functions so we can destructure nanoflakes.

That's legit something that I didn't consider at the time, but I think it might be useful to some people. I approve of it!

there are lot of getter like methods in Nanoflake. Idiomatic Kotlin would rather these simply be vals imo.

Considering that the Kotlin library was a port from Java, I totally agree that I didn't use the most idiomatic style of Kotlin.
In other hand, I'd like to consider each case (or class) being transformed in consideration first.

Regarding the three design points:

I agree to the first and second points. Value classes definitively weren't around at that time, and I think that the NanoflakeWithEpoch solution is also good.

In regard to the last point, although I don't see a issue by itself, wouldn't suspension functions require a dependency in kotlinx-coroutines? If so, I'd like to either avoid or implement a SuspendingNanoflakeGenerator in another library (either as a sub-library to this one or as another repository on the Nanoflakes organization)

Phew, I think that covers it all?
I really liked the points that you brought in and some overall improvements! Thank you and please feel free to ask more questions or open a PR!

@lost-illusi0n
Copy link

Awesome, great to hear we're on the same page.

In other hand, I'd like to consider each case (or class) being transformed in consideration first.

I don't think I fully understand what was asked. For the base Nanoflake I think the following changes would take place:

  • keep asLong() and asString()
  • all epoch related stuff goes away into NanoflakeWithEpoch
  • provide vals for timestamp, generator, and sequence that are backed by the appropriate Nanoflakes.xValue methods.

wouldn't suspension functions require a dependency in kotlinx-coroutines? ...

Yes, it contains the delay function.

If avoiding the dependency in the core implementation is the goal, I would go with a Gradle multi-module approach however that may change up artifact names (e.g, we would have nanoflakes-core and nanoflakes-suspending?) and I don't know how I feel about renaming the core artifact from nanoflakes-kotlin only for a small addition.

I'm not sure if this would warrant an entire new repository either though. Perhaps we just leave it as is for now?

Regarding everything else, I'm happy to start working on them and start sending PRs once they're ready.
Cheers!

@NotJustAnna
Copy link
Member Author

Answering @lost-illusi0n

I don't think I fully understand what was asked. For the base Nanoflake I think the following changes would take place:

* keep `asLong()` and `asString()`

* all epoch related stuff goes away into `NanoflakeWithEpoch`

* provide `val`s for `timestamp`, `generator`, and `sequence` that are backed by the appropriate `Nanoflakes.xValue` methods.

Oh, I see. I fully approve those changes!

If avoiding the dependency in the core implementation is the goal, I would go with a Gradle multi-module approach however that may change up artifact names (e.g, we would have nanoflakes-core and nanoflakes-suspending?) and I don't know how I feel about renaming the core artifact from nanoflakes-kotlin only for a small addition.

I'd like to avoid the dependency on Kotlin Coroutines, as well as a unnecessary dependency on kotlinx-datetime on Jvm and Js if possible.

I'd go with Gradle multi-modules as well! (Had forgotten the name of it so I called it sub-library in my previous comment)

IMO the coroutine-aware version could be called nanoflakes-kotlin-coroutines, and in this way we'd keep the core library as nanoflakes-kotlin.

Regarding everything else, I'm happy to start working on them and start sending PRs once they're ready.
Cheers!

You have my greenlight!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants