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 Table.union to the In-Memory Table. #4052

Merged
merged 27 commits into from
Jan 17, 2023

Conversation

radeusgd
Copy link
Member

Pull Request Description

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

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.

@radeusgd radeusgd requested a review from jdunkerley as a code owner January 14, 2023 17:13
@radeusgd radeusgd self-assigned this Jan 14, 2023
@radeusgd radeusgd force-pushed the wip/radeusgd/table-union-183854144 branch from ce37c49 to dcfabcd Compare January 16, 2023 12:27
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.

Generally looks good.
A few suggestions to clean up a little.

The union function is a little complicated to read so if we can break it up a little into helpers I think might be clearer but is good.

test/Table_Tests/src/Common_Table_Operations/Main.enso Outdated Show resolved Hide resolved
Comment on lines 1188 to 1189
is_everything_ok = tables_vector.all table->
if Table_Helpers.is_table table . not then Error.throw (Type_Error.Error Table table "tables") else
Copy link
Member

Choose a reason for hiding this comment

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

I'd be tempted to put this into Table_Helpers to make this read clearer as a lot going on in this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

Copy link
Member Author

Choose a reason for hiding this comment

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

The Table Helpers are primarily backend-agnostic and this is a backend-specific check, so instead I moved it into a private method in the Table module.

@radeusgd radeusgd force-pushed the wip/radeusgd/table-union-183854144 branch 2 times, most recently from 34fc258 to 46432e2 Compare January 16, 2023 20:47
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jan 16, 2023
@radeusgd radeusgd force-pushed the wip/radeusgd/table-union-183854144 branch from 3914dd5 to 474ecf9 Compare January 16, 2023 20:56
@mergify mergify bot merged commit 082e0bf into develop Jan 17, 2023
@mergify mergify bot deleted the wip/radeusgd/table-union-183854144 branch January 17, 2023 00:34
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