Skip to content

Use the BlockHeight type wherever it makes sense#2129

Merged
t-bast merged 1 commit intomasterfrom
block-height-everywhere
Jan 19, 2022
Merged

Use the BlockHeight type wherever it makes sense#2129
t-bast merged 1 commit intomasterfrom
block-height-everywhere

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented Jan 13, 2022

Follow up after #2113

@t-bast t-bast changed the title Use the BlockHeight wherever it makes sense Use the BlockHeight type wherever it makes sense Jan 13, 2022
@t-bast t-bast force-pushed the block-height-everywhere branch 2 times, most recently from 74c0c99 to 27ec8e0 Compare January 19, 2022 07:55
@t-bast t-bast marked this pull request as ready for review January 19, 2022 07:55
@t-bast t-bast requested a review from pm47 January 19, 2022 07:55
@t-bast t-bast force-pushed the block-height-everywhere branch 2 times, most recently from 2d48a54 to f282335 Compare January 19, 2022 09:15
We now have this better type to remove ambiguity.
We should use it wherever it makes sense.
There shouldn't be any business logic change in this commit.
@t-bast t-bast force-pushed the block-height-everywhere branch from f282335 to eda3ee1 Compare January 19, 2022 10:17
}

object BlockHeight {
def apply(underlying: Int): BlockHeight = BlockHeight(underlying.toLong)
Copy link
Member

Choose a reason for hiding this comment

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

Ha, should probably have done that for TimestampSecond and TimestampMilli.

Copy link
Member Author

Choose a reason for hiding this comment

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

Never too late! :)

Copy link
Member

Choose a reason for hiding this comment

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

Sure but then need to go over all tests to remove the L suffix.

@t-bast t-bast merged commit 58f9ebc into master Jan 19, 2022
@t-bast t-bast deleted the block-height-everywhere branch January 19, 2022 10:31
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