Add business days functions to Date and Date_Time

[radeusgd]

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/183082087

Important Notes

• Removed unnecessary invocations of Error.throw improving performance of Vector.distinct. The time of the add_work_days and work_days_until should be consistent with each other test suite came down from 15s to 3s after the changes.

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.

[radeusgd]

Seems that in some places callbacks were created by handler=(Error.throw ...) instead of handler=(_-> Error.throw ...) - this fired error creation regardless of if an error occurred - and this is costly as it generates a stack trace to attach to the error. Similarly, our Map was using dataflow-errors to implement get_or_else - so every such call would also generate a stack trace if the key was not present - again that's a lot of cases where dataflow errors were constructed without a need - I changed the code in such a way that get_or_else does not depend on catching dataflow errors.

[radeusgd]

I think in many places we use dataflow errors for 'control-flow'. I think this may be a relic of the fact that in the past dataflow errors did not have stack traces and were much faster to construct (just a hypothesis though). This has changed and now they incur a significant performance cost. I think we may consider adding a task to review their usage (I assume we want the stacktraces as they aid debugging) - maybe we should try to revisit our stdlib and check where we can replace dataflow errors with a cheaper mechanism. Or maybe we should give the user control over whether the dataflow error will get the stacktrace or not - then we could still have cheap dataflow errors for control flow and expensive ones for true error reporting. @jdunkerley @JaroslavTulach @hubertp what do you think?

[jdunkerley]

LGTM - couple of suggestions and one comment fix.

[jdunkerley]

I feel this calls for start.start_of Date_Period.Week

[radeusgd]

Well yes, but we did not implement the Week variant as you told me we want to postpone it because there are the complications with the "which day is the start of the week".

I avoided adding it to avoid adding additional features as I feel I sometimes tend to do that and then get the PR delayed because of it. So I decided to go for the simple solution - create a private helper which does only the job we need it to - being much simpler then the whole logic needed for start_of Date_Period.Week machinery.

Still - if you think this is the right time to implement start_of Date_Period.Week - I'm happy to do that. Do we want to add a start_of_week=Day_Of_Week.Monday setting which can be overridden?

[radeusgd]

Still I'd suggest doing it as a separate PR, just to move this forward and avoid mixing features in a single PR. Then we can replace these helper functions here in that next PR.

[jdunkerley]

Happy with that Get Outlook for iOS<https://aka.ms/o0ukef>

________________________________ From: Radosław Waśko ***@***.***> Sent: Wednesday, September 21, 2022 8:49:20 PM To: enso-org/enso ***@***.***> Cc: James Dunkerley ***@***.***>; Mention ***@***.***> Subject: Re: [enso-org/enso] Add business days functions to Date and Date_Time (PR #3726) @radeusgd commented on this pull request.

________________________________ In distribution/lib/Standard/Base/0.0.0-dev/src/Data/Time/Date.enso<#3726 (comment)>:

@@ -355,3 +504,39 @@ type Date

== self that = sign = Time_Utils.compare_to_localdate self that 0 == sign + +## PRIVATE +week_days_between start end = + ## We split the interval into 3 periods: the first week (containing the + starting point), the last week (containing the end point), and the full + weeks in between those. In some cases there may be no weeks in-between + and the first and last week can be the same week. + start_of_first_full_week = Time_Utils.start_of_next_week start Still I'd suggest doing it as a separate PR, just to move this forward and avoid mixing features in a single PR. Then we can replace these helper functions here in that next PR. — Reply to this email directly, view it on GitHub<#3726 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABD3MOMO4WQEWW7YF2MJMFTV7NREBANCNFSM6AAAAAAQRLYYHM>. You are receiving this because you were mentioned.Message ID: ***@***.***>