Skip to content

[Core] Replace TimeService with Java Time API #1620

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

Merged
merged 11 commits into from
May 30, 2019

Conversation

zutshiy
Copy link
Contributor

@zutshiy zutshiy commented May 4, 2019

Summary

Migrating the current code to use Java 8 Time API fields instead of the currently used primitive fields.

Details

The long fields time, timeStampMillis and duration have been replaced by Instant and Duration respectively.

These values can now be used instead of older primitives to allow for better representation of time and duration in cucumber without having to worry about precision or granularity.

Motivation and Context

To allow for more consistent time measurements and reduce custom code.

How Has This Been Tested?

All the JUnits have been updated accordingly to use Instant and Duration.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

zutshiy added 3 commits May 4, 2019 13:12
Migrating code to use Java 8 Time API for both Duration and Timestamps.

Using Instant and Duration for all instances.
@zutshiy zutshiy requested a review from mpkorstanje May 4, 2019 15:07
@zutshiy zutshiy self-assigned this May 4, 2019
Copy link
Contributor

@mleegwt mleegwt left a comment

Choose a reason for hiding this comment

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

Looks good to me. The JUnitFormatterTest contains changes in the time formatting. That change is not compatible with the previous behavior.is it correct?

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Looks good! I have reviewed about half now. I'll try to do the remainder next week. But feel free to make changes.

@zutshiy
Copy link
Contributor Author

zutshiy commented May 5, 2019

Looks good to me. The JUnitFormatterTest contains changes in the time formatting. That change is not compatible with the previous behavior.is it correct?

Correct. I updated the time formatting to handle for values received from Duration and Instant instead

@mpkorstanje
Copy link
Contributor

Correct. I updated the time formatting to handle for values received from Duration and Instant instead

I think that's because you no longer divide the nanos?

@zutshiy
Copy link
Contributor Author

zutshiy commented May 5, 2019

I think that's because you no longer divide the nanos?

Yes you're right. I had updated the formatter to use nanos directly so I updated the tests as well. But I am updating the format back to millis now and adding the division.

@zutshiy zutshiy changed the title Java 8 Time API Migration [WIP] Java 8 Time API Migration May 26, 2019
@mpkorstanje
Copy link
Contributor

Looks pretty good so far! I'll need a bit more time to check the details.

@zutshiy zutshiy changed the title [WIP] Java 8 Time API Migration Java 8 Time API Migration May 30, 2019
@mpkorstanje mpkorstanje changed the title Java 8 Time API Migration [Core] Replace TimeService with Java Time API May 30, 2019
@mpkorstanje mpkorstanje merged commit 4bb8876 into cucumber:develop-v5 May 30, 2019
@mpkorstanje
Copy link
Contributor

Looks all good. Cheers!

@zutshiy zutshiy deleted the java8-timeapi-migration branch May 31, 2019 05:38
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.

3 participants