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 key functions to Table to make it act as [Column] #3644

Merged
merged 8 commits into from
Aug 18, 2022

Conversation

radeusgd
Copy link
Member

Pull Request Description

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

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.

Comment on lines -426 to +433
new_index = (Helpers.unify_vector_singleton index).map (self.at >> .as_internal)
new_index = (Helpers.unify_vector_singleton index).map ((self.at _) >> .as_internal)
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 default parameter to at (being 0 to choose first available column), but this has a downside that now we cannot nicely use table.at as a higher order function - if it gets no arguments it will use the defaults - so to pass it as an unapplied function we need to use the _ (or in the future ... or sth) to make sure defaults are not triggered.

I'm not saying this is a problem, especially as our GUI users will probably not use at in this mode too often. Knowing that our rule of thumb is to have default arguments wherever possible, I think we may keep it, but there is a trade-off to make.

Another thing I'm worried about is that if some users start learning about higher order functions they will can really easily get confused with how these default arguments work - but this a bigger problem of language design and I'm not sure if a better solution than we currently have is possible - it is always a trade-off. We probably just need to be careful about these defaults when writing user manuals, to make sure the users understand that they need the underscore to avoid defaults. We may also consider trying to improve the error messages somehow, as currently tracking the Not_Invokable_Error was a bit hard even for me, who I tihnk knows Enso pretty well.

@radeusgd radeusgd force-pushed the wip/radeusgd/table-at-integer-181370836 branch from 16c5474 to c99f5fa Compare August 12, 2022 08:53
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Aug 18, 2022
@mergify mergify bot merged commit bcca7f1 into develop Aug 18, 2022
@mergify mergify bot deleted the wip/radeusgd/table-at-integer-181370836 branch August 18, 2022 12:33
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