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

Define Enso epoch start as 15th October 1582. #3804

Merged
merged 9 commits into from
Oct 27, 2022

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Oct 17, 2022

Pull Request Description

Define start of Enso epoch as 15th of October 1582 - start of the Gregorian calendar.

Important Notes

  • Some (Gregorian) calendar related functionalities within Date and Date_Time now produces a warning if the receiving Date/Date_Time is before the epoch start, e.g., week_of_year, is_leap_year, etc.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@Akirathan Akirathan force-pushed the wip/akirathan/epoch-start-183102477 branch from f917a3f to 854ce25 Compare October 19, 2022 12:56
@Akirathan Akirathan force-pushed the wip/akirathan/epoch-start-183102477 branch 2 times, most recently from 0cd2cd4 to c356d39 Compare October 19, 2022 13:02
@Akirathan Akirathan marked this pull request as ready for review October 19, 2022 13:02
@Akirathan Akirathan self-assigned this Oct 19, 2022
@Akirathan Akirathan force-pushed the wip/akirathan/epoch-start-183102477 branch from c356d39 to 3852867 Compare October 19, 2022 17:11
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Code looks good to me overall, although the tests need to be amended because currently they do not test stuff.

I also have a few suggestions how to make things a bit clearer (in my subjective opinion), however the current way is also mostly good. The only strong thing is the big workdays functions - I think in the current form it is hard to keep track if all codepaths correctly check for the epoch and I'd like that to be clear to avoid bugs.

One last thing - from the earlier discussions I was not sure if we want to return the dataflow error for these operations. My initial understanding was that we allow to return a result, but attach a dataflow warning to such an operation saying that the result cannot be trusted. This is a bit more permissive as the user can still use the operations, they are just warned that results may be wrong. It is a bit more dangerous as it's easier to remove the warnings though. I have no opinion on this, @jdunkerley what was the idea here? Do we want dataflow errors or warnings?

@Akirathan
Copy link
Member Author

One last thing - from the earlier discussions I was not sure if we want to return the dataflow error for these operations. My initial understanding was that we allow to return a result, but attach a dataflow warning to such an operation saying that the result cannot be trusted. This is a bit more permissive as the user can still use the operations, they are just warned that results may be wrong. It is a bit more dangerous as it's easier to remove the warnings though. I have no opinion on this, @jdunkerley what was the idea here? Do we want dataflow errors or warnings?

The original idea was to use warnings. I will change the behavior so that the methods that fail with an exception in the underlying java methods will fail with an error, and all the other methods that now fail with error, will just attach a warning instead. So that, e.g., trying to get number of days for a Date_Time that is before the epoch will attach a warning and won't fail with an error.

@jdunkerley
Copy link
Member

One last thing - from the earlier discussions I was not sure if we want to return the dataflow error for these operations. My initial understanding was that we allow to return a result, but attach a dataflow warning to such an operation saying that the result cannot be trusted. This is a bit more permissive as the user can still use the operations, they are just warned that results may be wrong. It is a bit more dangerous as it's easier to remove the warnings though. I have no opinion on this, @jdunkerley what was the idea here? Do we want dataflow errors or warnings?

Yes the suggestion was to return the values with a warning attached. This would allow the users to perform the computation but warn that the values are possibly wrong.

@Akirathan Akirathan force-pushed the wip/akirathan/epoch-start-183102477 branch 2 times, most recently from d3bcd77 to 0a5500c Compare October 24, 2022 16:33
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

LGTM re engine changes

Copy link
Member

@radeusgd radeusgd 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, but I think it would be good to wrap the warnings in some Atom instead of using raw texts. Plus a few very minor nitpicks.

@Akirathan Akirathan force-pushed the wip/akirathan/epoch-start-183102477 branch from 0a5500c to 8ba80ca Compare October 27, 2022 09:29
@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Oct 27, 2022
@mergify mergify bot merged commit 28243a0 into develop Oct 27, 2022
@mergify mergify bot deleted the wip/akirathan/epoch-start-183102477 branch October 27, 2022 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants