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

Add startTime and endTime in OffsetDateTime format #9

Merged
merged 1 commit into from
Jan 9, 2023
Merged

Add startTime and endTime in OffsetDateTime format #9

merged 1 commit into from
Jan 9, 2023

Conversation

doljae
Copy link
Contributor

@doljae doljae commented Jan 7, 2023

Motivation:

  • We can check the start time and end time in the log for condition results.
  • The unit of this start time and end time is milliseconds. This is a bit difficult to read from a user's point of view.

Modifications:

  • Add startTime and endTime in OffsetDateTime format
  • Add related methods, change variable names

Result:

  • Now we can also see the start and end times in the logs in normal time notation instead of milliseconds.
  • It is expected that the user will be able to check the information related to the start of the condition more intuitively.
  • In ConditionMatchResult, you can check the OffsetDateTime type value similarly to the way you check the startTime and endTime.

Additional info:

  • If startTime and endTime in milliseconds is the intended function please comment 🙂

Comment on lines 159 to 164
/**
* Returns {@link OffsetDateTime} from the {@link ConditionMatchResult#startTimeMillis}.
*/
public OffsetDateTime startTime() {
return Instant.ofEpochMilli(startTimeMillis).atZone(ZoneId.systemDefault()).toOffsetDateTime();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the great suggestion! In my opinion, this method is not necessary. Because the conversion from milliseconds to OffsetDateTime doesn't have to be done here. Because developers using Conditional can do it themselves. What do you think of using the method below??

private static String millisAsISO8601String(long millis) {
    return Instant.ofEpochMilli(millis).atZone(ZoneId.systemDefault()).toOffsetDateTime().toString();
}

Copy link
Contributor Author

@doljae doljae Jan 8, 2023

Choose a reason for hiding this comment

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

So, summarize your opinion...

  1. ConditionMatchResult.startTimeMillis, ConditionMatchResult.endTimeMillis properties remain as long type as before.
  2. ConditionMatchResult.toString() outputs startTime and endTime in OffsetDateTime format.

This seems like a good way. The values for startTime and endTime can be obtained through the public methods startTimeMillis() and endTimeMillis(), and We can know that the returned values are milliseconds through these method names, so users can return them as appropriate Date objects as needed 🙂

However, I think it is something to consider about the mismatch between the type provided in the log(OffsetDateTime) and the type actually provided(long).

Also, to use OffsetDateTime, the user must separately call the same method(you suggested) used to convert long to OffsetDateTime inside ConditionMatchResult.

So, if there's conversion from long to OffsetDateTime inside ConditionMatchResult.toString(), I think it's okay to add this value as a field and provide it for use as a public method.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we don't have to provide a public method to convert from Milliseconds -> OffsetDateTime. Because we need ZoneId or ZoneOffset to convert from Milliseconds to OffsetDateTime. Then, we need to receive ZoneId or ZoneOffset as parameter, which is no different from converting Milliseconds -> OffsetDateTime directly by the developer.

Copy link
Contributor Author

@doljae doljae Jan 8, 2023

Choose a reason for hiding this comment

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

Thank you for your kind explanation.
As you said, it seems more reasonable to provide the millisecond information as before, and let the user use this to get the necessary Date object.

I'll fix on it based on what you commented on 🙂

Copy link
Contributor Author

@doljae doljae Jan 9, 2023

Choose a reason for hiding this comment

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

I applied the way you suggested at 0fa5717 🙂

Comment on lines 207 to 208
.append(", startTimeMillis=").append(millisAsString(startTimeMillis))
.append(", endTimeMillis=").append(millisAsString(endTimeMillis))
Copy link
Contributor

Choose a reason for hiding this comment

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

If startTime and endTime are displayed in ISO-8601 format, it would be good to remove startTimeMillis and endTimeMillis.

Copy link
Contributor Author

@doljae doljae Jan 9, 2023

Choose a reason for hiding this comment

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

Same as #9 (comment)

Copy link
Contributor

@icepeppermint icepeppermint left a comment

Choose a reason for hiding this comment

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

Great work! Thank you for your contribution!

@icepeppermint icepeppermint merged commit 737b562 into line:main Jan 9, 2023
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.

None yet

2 participants