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 Date_Range #6621

Merged
merged 21 commits into from
May 11, 2023
Merged

Add Date_Range #6621

merged 21 commits into from
May 11, 2023

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented May 9, 2023

Pull Request Description

Closes #6543

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@radeusgd radeusgd force-pushed the wip/radeusgd/6543-date-range branch 3 times, most recently from 125fe9a to c6f9e2f Compare May 10, 2023 19:51
Comment on lines +456 to +487
## PRIVATE
Assumes that the range is not empty.
compute_length_step_months start end step increasing =
diff = case increasing of
True -> Time_Utils.months_between start end
False -> Time_Utils.months_between end start
# assert (diff >= 0)
steps = diff . div step
exact_fit = case increasing of
True -> start + Period.new months=steps*step == end
False -> start - Period.new months=steps*step == end
if exact_fit then steps else steps+1
Copy link
Member Author

Choose a reason for hiding this comment

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

I was a bit skeptical about this heuristic, but I added as many test cases trying to breaking as I could imagine and it works, so I think we should be fine. The bright side is that it gives us O(1) time complexity for the case where the Period has days=0. So the brute force O(N) is now only when both days and months are non-zero.

Apparently, the JDK uses a pretty smart way to compute the month difference:

    private long monthsUntil(LocalDate end) {
        long packed1 = getProlepticMonth() * 32L + getDayOfMonth();  // no overflow
        long packed2 = end.getProlepticMonth() * 32L + end.getDayOfMonth();  // no overflow
        return (packed2 - packed1) / 32;
    }

(cited from LocalDate.java)

@radeusgd radeusgd marked this pull request as ready for review May 10, 2023 21:12
@radeusgd radeusgd self-assigned this May 10, 2023
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Looks really good - a couple of minor things.

Comment on lines +36 to +37
1.up_to 1 include_end=True . to_vector . should_equal [1]
1.down_to 1 include_end=True . to_vector . should_equal [1]
Copy link
Member

Choose a reason for hiding this comment

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

worth testing 1.up_to 0 include_end=True?

## PRIVATE
Never use the constructor directly to construct a range, as it does not
allow to verify invariants and may lead to unexpected behavior.
Internal_Constructor (start : Date) (end : Date) (step : Period) (increasing : Boolean) (cached_length : Integer)
Copy link
Member

Choose a reason for hiding this comment

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

Should we call it Between (I know we want to avoid use) but it gets rendered sometimes and visible to user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I really wanted to mark it Internal as using it ever is going to cause fires.

Where ever can it render? I specifically ensured we override to_text and to_display_text, ooh I guess I should check pretty too - will do and will add tests.

Ah and I guess to_js_object - I will amend it to have a sane name and skip the internal fields.

The only remaining place I can think of is Meta but that is rather advanced and internal stuff anyway so I think we can ignore it there.

@radeusgd radeusgd force-pushed the wip/radeusgd/6543-date-range branch from 457bbb1 to fd4541f Compare May 11, 2023 12:59
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label May 11, 2023
@mergify mergify bot merged commit cd7fb73 into develop May 11, 2023
@mergify mergify bot deleted the wip/radeusgd/6543-date-range branch May 11, 2023 16:03
Procrat added a commit that referenced this pull request May 12, 2023
* develop:
  Implement loading spinner for visualisations. (#6512)
  Fix blank input port (#6614)
  Add `Date_Range` (#6621)
  All Vector operations shall be applicable on java.util.ArrayList (#6642)
  Fix redirect paths and enable authentication and new dashboard by default (#6605)
  Fix #6287: wrong nested breadcrumb ordering (#6617)
  Whitelist AWS Cognito domains (#6643)
  Revert "Add COOP+COEP+CORP headers (#6597)" (#6647)
  Fix shortcuts table formatting (#6644)
  Automatic type based dropdown does not include singleton in a union type (#6629)
  Make Meta.get_annotation work for constructor (#6633)
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.

Range.Between not working with Dates
3 participants