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

Handle Nothing values in Filter_Condition.to_predicate #8600

Merged
merged 11 commits into from
Dec 21, 2023

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Dec 20, 2023

Pull Request Description

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 added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 20, 2023
@radeusgd radeusgd self-assigned this Dec 20, 2023
Comment on lines +202 to +212
Equal_Ignore_Case value locale ->
handle_nothing <| txt-> (txt : Text).equals_ignore_case value locale
Starts_With prefix case_sensitivity ->
handle_nothing <| txt-> (txt : Text).starts_with prefix case_sensitivity
Ends_With suffix case_sensitivity ->
handle_nothing <| txt-> (txt : Text).ends_with suffix case_sensitivity
Contains substring case_sensitivity ->
handle_nothing <| txt-> (txt : Text).contains substring case_sensitivity
Not_Contains substring case_sensitivity ->
handle_nothing <| txt-> (txt : Text).contains substring case_sensitivity . not
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 added the txt:Text checks, because I think that Expected txt to be Text, but got [...]. will be more readable than e.g. Method .equals_ignore_case of type [...] could not be found..

Comment on lines +318 to +325
txtvec = alter ["abab", "baaa", Nothing, "cccc", "BAAA"]
txtvec.filter (Filter_Condition.Equal_Ignore_Case "baaA") . should_equal ["baaa", "BAAA"]
txtvec.filter (Filter_Condition.Contains "a") . should_equal ["abab", "baaa"]
txtvec.filter (Filter_Condition.Starts_With "a") . should_equal ["abab"]
txtvec.filter (Filter_Condition.Ends_With "a") . should_equal ["baaa"]
txtvec.filter (Filter_Condition.Like "b%a") . should_equal ["baaa"]
# Nothing is not included in the negation either
txtvec.filter (Filter_Condition.Not_Like "b%a") . should_equal ["abab", "cccc", "BAAA"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Note, here we 'break' the property that

forall v p,
(v.filter (Filter_Condition.Like p)) + (v.filter (Filter_Condition.Not_Like p)) . length == v.length

because the Nothing values do not get into either category.

I think that is OK, because Like and Not_Like are both Text filters that return _Text_s that satisfy a condition, Nothing is neither like nor not-like a pattern.

Do you think that is OK?

I'm also fine with including Nothing in the Not_Like case if that is preferred.

@radeusgd radeusgd force-pushed the wip/radeusgd/8549-vector-filter-allow-nulls branch from 75d638e to 31136bd Compare December 20, 2023 15:38
@radeusgd radeusgd force-pushed the wip/radeusgd/8549-vector-filter-allow-nulls branch from 31136bd to 88a5da6 Compare December 21, 2023 08:54
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Dec 21, 2023
@mergify mergify bot merged commit b3de42e into develop Dec 21, 2023
29 of 30 checks passed
@mergify mergify bot deleted the wip/radeusgd/8549-vector-filter-allow-nulls branch December 21, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vector.filter by Filter_Condition does not work well with Nothing
2 participants