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 Reexport Field to Suggestions #1793

Merged
merged 29 commits into from
Jul 20, 2021
Merged

Add Reexport Field to Suggestions #1793

merged 29 commits into from
Jul 20, 2021

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Jun 10, 2021

Pull Request Description

close #1670

PR adds the reexport field to suggestions. It contains a fully qualified module name of the outermost module (module with the shortest path) that re-exports the suggestion.

Changelog:

  • feat: add new suggestion type Module. Previously modules were represented as atom suggestions
  • feat: add reexport field to suggestions
  • feat: extract the list of re-exported symbols from a module during compilation
  • feat: implement diff algorithm between exported symbols of two modules (before and after edit)
  • feat: apply updates with module exports to suggestions database
  • misc: suggestions handler applies suggestion database updates in a sequence
  • misc: update SQLite dependency

Important Notes

⚠️ PR is not backward-compatible with IDE because of the new suggestion variant Module

Changes in the Language Server protocol.

  1. Added new Suggestion type: Module
 // A type of suggestion entries.
 type SuggestionEntry =
+  // A module
+  | SuggestionEntryModule
   // A value constructor
   | SuggestionEntryAtom
   // A method defined on a type
@@ -409,6 +411,12 @@ type SuggestionEntry =
   // A local value
   | SuggestionEntryLocal;

+interface SuggestionEntryModule {
+  module: string;
+  documentation?: string;
+  reexport?: string;
+}
  1. Module, Atom and Mehod suggestions have an optional reexport field.
interface SuggestionEntryAtom {
   externalId?: UUID;
   name: string;
@@ -416,6 +424,7 @@ interface SuggestionEntryAtom {
   arguments: SuggestionEntryArgument[];
   returnType: string;
   documentation?: string;
+  reexport?: string;
 }

 interface SuggestionEntryMethod {
@@ -426,6 +435,7 @@ interface SuggestionEntryMethod {
   selfType: string;
   returnType: string;
   documentation?: string;
+  reexport?: string;
 }
  1. reexport field can be modified.
/**
 * The kind of the suggestions database update.
 */
type SuggestionsDatabaseUpdate = Add | Remove | Modify;

@@ -675,6 +796,11 @@ interface Modify {
    * The scope to update.
    */
   scope?: FieldUpdate<SuggestionEntryScope>;
+
+  /**
+   * The reexport field to update.
+   */
+  reexport?: FieldUpdate<String>;
 }

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 documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@4e6 4e6 added Type: Enhancement p-high Should be completed in the next sprint labels Jun 10, 2021
@4e6 4e6 self-assigned this Jun 10, 2021
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.

Looks good to me, but in some places additional documentation explaining the mechanisms would be helpful to understand the intentions behind the code

docs/language-server/protocol-language-server.md Outdated Show resolved Hide resolved
docs/language-server/protocol-language-server.md Outdated Show resolved Hide resolved
Comment on lines +727 to +729
suggestionUpdatesQueue: mutable.Queue[
Api.SuggestionsDatabaseModuleUpdateNotification
] = mutable.Queue.empty,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this queue mutable? It feels like an anti-pattern to store a mutable queue in a state that is meant to be immutable and is explicitly modified. If the queue is to be mutable, it could just as well be stored in a member variable of the object instead of this state, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, in general, a mutable state in actor it's an antipattern and can be refactored by making it an argument of a receive function. This works well with single values, but this case is a bit different. On startup, we receive a burst of suggestion updates. Updating a mutable queue is more efficient than copying the queue on every update.

@4e6 4e6 merged commit 980ba8c into main Jul 20, 2021
@4e6 4e6 deleted the wip/db/suggestions-reexport-1 branch July 20, 2021 16:10
iamrecursion pushed a commit that referenced this pull request Jul 23, 2021
Add the reexport field to suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-high Should be completed in the next sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Re-exported Methods to the Suggestions Database
3 participants