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.

Sign up for GitHub

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

Jump to bottom

Experimenta kotlin.time.Duration support #1811

Closed
wants to merge 3 commits into from
Closed

Experimenta kotlin.time.Duration support #1811

wants to merge 3 commits into from

Conversation

fvasco
Copy link
Contributor

@fvasco fvasco commented Feb 16, 2020

Initial proposal, no test included.
Fixes #1402

@elizarov
Copy link
Contributor

Look good. Needs tests. Also, these functions should not be inline. There's no much gain from inlining here, plus it would limit further evolution. For example, we could make them nanosecond-precise in the future (if anyone needs it) without recompilation or deprecated long-accepting version when Duration becomes stable without having references to it in the bytecode.

@fvasco
Copy link
Contributor Author

fvasco commented Feb 17, 2020

Hi @elizarov,
I agree with you.
I marked these functions inline due to its experimental nature. I will remove the modifier.

Should I put all tests in the DelayTest class, should I duplicate the DelayTest class, should I create two different ones with a common abstract superclass? Any suggestion?

@elizarov
Copy link
Contributor

I'd duplicate DelayTest into something like DelayDurationTest.

@fvasco fvasco force-pushed the 1402-duration branch 2 times, most recently from ab31438 to 90e3285 Compare February 17, 2020 18:29
@fvasco
Copy link
Contributor Author

fvasco commented Feb 17, 2020

Done.

@EdwarDDay
Copy link
Contributor

Do you also want to add kotlin.time.Duration support to the corresponding Flow operators (sample and debounce)?

@fvasco
Copy link
Contributor Author

fvasco commented Feb 27, 2020

Good idea

elizarov
elizarov suggested changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@elizarov elizarov left a comment

Choose a reason for hiding this comment

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

Minor detail, otherwise great.

kotlinx-coroutines-core/common/src/Delay.kt Outdated Show resolved Hide resolved
@qwwdfsad qwwdfsad force-pushed the develop branch 3 times, most recently from 4a49830 to aff8202 Compare March 10, 2020 17:27
@elizarov elizarov mentioned this pull request Mar 13, 2020
Merged
elizarov added a commit that referenced this pull request Mar 13, 2020
@elizarov @fvasco
kotlin.time.Duration support (#1811)
1e7ca75 
Fixes #1402

Co-authored-by: Francesco Vasco <fsco_v-github@yahoo.it>
@elizarov
Copy link
Contributor

Merged with #1864

@elizarov elizarov closed this Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyoleg whyoleg whyoleg left review comments

@elizarov elizarov elizarov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fvasco @elizarov @EdwarDDay @whyoleg