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

Update starts_with, ends_with and contains to new API design #4078

Merged
merged 20 commits into from
Jan 25, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jan 24, 2023

Pull Request Description

  • Updated Text.starts_with, Text.ends_with and Text.contains to new simpler API.
  • Added a Case_Sensitivity.Default and adjusted Table.distinct to use it by default.
  • Fixed a bug with Data.fetch on an HTTP error.
  • Improved SQLite Case Sensitivity control in distinct to use collations.

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.

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.

Awesome!

I really like the simplicity of the new API and also the fact that we now express default handling by an explicit Atom instead of Nothing.

Just one code style suggestion - I think the locale check should now become the responsibility of prepare_distinct.

I'd add a test checking it and mark it pending in Postgres - so that we have it clearly documented that this feature is pending. I think it is worth keeping such 'documentation-test' as if we get some bug report in the future at least we'll be more aware that this was a conscious decision to defer this instead of being confused that it is a bug.

And I think we should be explicit that forcing case sensitivity in Postgres is currently not supported - otherwise a user may use it and be confused why it doesn't work. Especially as this workaround does not affect the default usage.

Nothing ->
Case_Sensitivity.Default ->
Copy link
Member

Choose a reason for hiding this comment

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

I'm so fond of this change! Meaningful constructors are always much better over Nothing carrying thousands of meanings.

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jan 24, 2023
@mergify mergify bot merged commit 60f0e96 into develop Jan 25, 2023
@mergify mergify bot deleted the wip/jd/defect-work branch January 25, 2023 11:47
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.

2 participants