-
Notifications
You must be signed in to change notification settings - Fork 82
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
Fixes and improvements for Snowflake #370
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
76a35b6
Fix Snowflake constructor taking an Instant (subtract milliseconds be…
lukellmann b0bafb8
Fix SnowflakeMark.elapsedNow returning Duration with wrong sign (befo…
lukellmann 86d1412
Fix Snowflake.timeMark passing unadjusted milliseconds since Discord …
lukellmann c47caa0
Deduplicate timeStamp creation logic in Snowflake.timeStamp and Snowf…
lukellmann ad0d746
Using ushr instead of shr in Snowflake to make timestamp related func…
lukellmann 9c6ba3d
Additional documentation for compareTo and equals behavior for Snowfl…
lukellmann 3f9ad82
Snowflake: tests, checks in constructor that takes instant, endOfTime…
lukellmann aa55190
Fix test naming
lukellmann ab3a2f3
Fix test naming again
lukellmann c889ff9
Use ULong for Snowflake value
lukellmann 74c91bf
Change Snowflake.Serializer from class to object
lukellmann 361b87a
Add pseudo Snowflake constructor that takes a Long value
lukellmann b0262aa
Don't use java.math.BigInteger for Test
lukellmann 8f06f0c
Remove unnecessary range for Random.nextULong()
lukellmann File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
package entity | ||
|
||
import dev.kord.common.entity.Snowflake | ||
import kotlinx.datetime.Clock | ||
import kotlinx.datetime.DateTimeUnit.Companion.MILLISECOND | ||
import kotlinx.datetime.Instant | ||
import kotlinx.datetime.minus | ||
import kotlinx.datetime.plus | ||
import kotlin.test.* | ||
|
||
class SnowflakeTest { | ||
|
||
@Test | ||
fun `Snowflake with value ULong MIN_VALUE has timeStamp equal to discordEpochStart`() { | ||
val snowflake = Snowflake(ULong.MIN_VALUE) | ||
assertEquals(Snowflake.discordEpochStart, snowflake.timeStamp) | ||
} | ||
|
||
@Test | ||
fun `Snowflake with value ULong MAX_VALUE has timeStamp equal to endOfTime`() { | ||
val snowflake = Snowflake(ULong.MAX_VALUE) | ||
assertEquals(Snowflake.endOfTime, snowflake.timeStamp) | ||
} | ||
|
||
@Test | ||
fun `Snowflake created from instant far in the past has timeStamp equal to the timeStamp of Snowflake min`() { | ||
val snowflake = Snowflake(Instant.DISTANT_PAST) | ||
assertEquals(Snowflake.min.timeStamp, snowflake.timeStamp) | ||
} | ||
|
||
@Test | ||
fun `Snowflake created from instant far in the future has timeStamp equal to the timeStamp of Snowflake max`() { | ||
val snowflake = Snowflake(Instant.DISTANT_FUTURE) | ||
assertEquals(Snowflake.max.timeStamp, snowflake.timeStamp) | ||
} | ||
|
||
@Test | ||
fun `Snowflake's timeStamp calculates an Instant close to the Instant the Snowflake was created from`() { | ||
val instant = Clock.System.now() | ||
val snowflake = Snowflake(instant) | ||
|
||
// snowflake timestamps have a millisecond accuracy -> allow +/-1 millisecond from original instant | ||
val validTimeRange = instant.minus(1, MILLISECOND)..instant.plus(1, MILLISECOND) | ||
|
||
assertContains(validTimeRange, snowflake.timeStamp) | ||
} | ||
|
||
@Test | ||
fun `min Snowflake's timeMark has passed`() { | ||
assertTrue(Snowflake.min.timeMark.hasPassedNow()) | ||
} | ||
|
||
@Test | ||
fun `max Snowflake's timeMark has not passed`() { | ||
assertFalse(Snowflake.max.timeMark.hasPassedNow()) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest retaining a
Long
constructor for ease of use and migration, I imagine plenty of people have aSnowflake(long)
somewhere defined as a constant and we're a far ways off from reaching the most significant bit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will add this, but two decisions are needed:
ULong
with the same binary representation (meaning -1 will give aSnowflake
with maximum timestamp)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be ok to throw an exception if I add
@Deprecated
to the pseudo constructor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a top-level factory function. We're trying to move away from companion invokes ever since the naming rules for factory function changed.
The scenario shouldn't happen all that, so I'm not really concerned with breaking changes. However, negative values were valid (although undocumented) before this change. It might be best to just handle them as unsigned values as well.
I wouldn't want it to be marked Deprecated, the whole point of adding the faux constructor is convenience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done