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

Short hand version for order_by #3643

Merged
merged 5 commits into from
Aug 16, 2022

Conversation

radeusgd
Copy link
Member

Pull Request Description

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

Important Notes

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.

type By_Column (columns : Vector Sort_Column.Column)
type By_Name (columns : Vector (Sort_Column.Name | Text)) (matcher:Matcher=Text_Matcher)
type By_Index (columns : Vector (Sort_Column.Index | Integer))
type By_Column (columns : Vector (Sort_Column.Column | Column))
Copy link
Member Author

Choose a reason for hiding this comment

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

It is a bit worrisome to me that the Column type actually does not exist! So we are lying a bit here and it may bite us once we have some validation of these type signatures.

Well - we have a Column type but there is one for Table and another for Database and without interfaces we cannot really relate them. In the absence of typechecking, maybe we can go for Table.Column, but in the longer term we probably need to replace this with some Column interface. This is a common issue across the Table/Database library.

Copy link
Member

Choose a reason for hiding this comment

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

I agree - this is a common issue and one the revised type system will need to resolve.

Pinging @ekmett - one to consider on the stuff you are working on.

@radeusgd radeusgd force-pushed the wip/radeusgd/order-by-shorthand-182868310 branch from cec3c83 to 1a1dd3d Compare August 12, 2022 08:53
@jdunkerley jdunkerley force-pushed the wip/radeusgd/order-by-shorthand-182868310 branch from 1a1dd3d to 00573ab Compare August 16, 2022 13:07
@jdunkerley jdunkerley force-pushed the wip/radeusgd/order-by-shorthand-182868310 branch from 00573ab to 3d9af6d Compare August 16, 2022 13:10
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Aug 16, 2022
@mergify mergify bot merged commit fbf6c80 into develop Aug 16, 2022
@mergify mergify bot deleted the wip/radeusgd/order-by-shorthand-182868310 branch August 16, 2022 15:41
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.

None yet

2 participants