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

Split Atom suggestion entry to Type and Constructor #3835

Merged
merged 18 commits into from
Nov 2, 2022

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Oct 28, 2022

Pull Request Description

Changelog:

  • update: split Atom suggestion to Type and Constructor
  • update: gui API
  • update: JSONRPC doc

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 498 to 499
/** The type of an atom. */
returnType: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the returnType of the Type? I assume it will be always the type? I would left this field in Constructor, here it does not make much sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, it can be re-constructed from the module name and the type name. Removed

Comment on lines 518 to 522
/** The atom name. */
name: string;

/** The module name where the atom is defined. */
module: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Docs still use atom term.
  2. Isn't module entirely deducible from returnType field? Is there a possibility of "Extension constructors"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I updated all atom references in the doc.

The module can probably be deducible from returnType right now. But later when we start supporting more complex types, it might not be the case. I'd leave this field as is

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

The parts on the engine side look fine from my perspective. I suggest to get a review from @kustosz - he can judge the best whether SuggestionBuilder records the properties of the language properly.

Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Minor nits

Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

I have some reservations regarding Type having a returnType, and, in particular, the returnType being the type itself. This is only true for no-constructor types, such as Nothing. For types that have constructors, the type itself has a singleton type (containing the static methods). There isn't a notation for this singleton type AFAIK, but probably one should be made up. Otherwise we run the risk of IDE providing wrong suggestions. If a method expects a Maybe, the word Maybe is not an acceptable argument. So we shouldn't tell them that Maybe: Maybe

@4e6
Copy link
Contributor Author

4e6 commented Oct 31, 2022

So we shouldn't tell them that Maybe: Maybe

The returnType is only used internally and not exposed to the IDE in the JSONRPC protocol. We need it as an index in the suggestions database. And it is much cheaper to store it in a field than to compute it.

Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

The Rust part LGTM.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Nov 2, 2022
@mergify mergify bot merged commit a6ce49e into develop Nov 2, 2022
@mergify mergify bot deleted the wip/db/suggestion-atom-constructor branch November 2, 2022 09:53
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

5 participants