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.

Sign up for GitHub

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

Jump to bottom

Search suggestions by static attribute #6036

Merged
merged 7 commits into from
Mar 23, 2023
Merged

Search suggestions by static attribute #6036

merged 7 commits into from
Mar 23, 2023

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Mar 22, 2023

Pull Request Description

close #5874

Changelog:

  • add: isStatic parameter to search/completion request to search by the static suggestion attribute
  • update: search non-static suggestions when opening component browser

Important Notes

Component browser doesn't show Table.new and Table.from_rows suggestions when a Table node is selected.

2023-03-21-151117_1301x877_scrot

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 22, 2023
@4e6 4e6 self-assigned this Mar 22, 2023
farmaazon
farmaazon requested changes Mar 22, 2023
View reviewed changes
app/gui/src/controller/searcher.rs Outdated Show resolved Hide resolved
farmaazon
farmaazon reviewed Mar 22, 2023
View reviewed changes
app/gui/src/controller/searcher.rs Outdated
@@ -1218,7 +1218,8 @@ impl Searcher {
let requests = return_types_for_engine.into_iter().map(|return_type| {
info!("Requesting suggestions for returnType {return_type:?}.");
let file = graph.module.path().file_path();
ls.completion(file, &position, &this_type, &return_type, &tags)
let is_static = if this_type.is_some() { Some(false) } else { None };
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put it outside the closure (as I suggested) - there is no need to recompute it for every return_type.

Also, the if may be replaced with then_some method: https://doc.rust-lang.org/std/primitive.bool.html#method.then_some

JaroslavTulach
JaroslavTulach approved these changes Mar 22, 2023
View reviewed changes
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.

Please describe what is the default value of the isStatic attribute - otherwise OK.

app/gui/controller/engine-protocol/src/language_server.rs
@@ -177,6 +177,7 @@ trait API {
, self_type : Option<String>
, return_type : Option<String>
, tags : Option<Vec<SuggestionEntryType>>
, is_static : Option<bool>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the attribute have a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the parameters of a search request. If you don't want to specify a parameter, you pass None

docs/language-server/protocol-language-server.md
@@ -4368,6 +4368,8 @@ Sent from client to the server to receive the autocomplete suggestion.
returnType?: string;
// Filter by the suggestion types
tags?: [SuggestionEntryType];
// Filter by `static` attribute of the suggestion
isStatic?: Boolean;
Copy link
Member

Choose a reason for hiding this comment

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

What is the default value when the attribute is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the TypeScript ? notation in the type definition, i.e.

{
    a?: Type;
}

means that the a attribute is optional and may be null

engine/language-server/src/main/scala/org/enso/languageserver/search/SearchApi.scala
@@ -76,7 +76,8 @@ object SearchApi {
position: Position,
selfType: Option[String],
returnType: Option[String],
tags: Option[Seq[SuggestionKind]]
tags: Option[Seq[SuggestionKind]],
isStatic: Option[Boolean]
Copy link
Member

Choose a reason for hiding this comment

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

Can one ask for both - static/instance - methods at once? Or one needs two queries?

Or is it never needed to ask for both types at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can one ask for both - static/instance - methods at once?

If you don't care about the static attribute, you just pass None. This way the method will return both static and non-static suggestions.

@4e6 4e6 requested a review from farmaazon March 22, 2023 18:35
@enso-bot enso-bot bot mentioned this pull request Mar 23, 2023
Closed
@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Mar 23, 2023
hubertp
hubertp approved these changes Mar 23, 2023
View reviewed changes
lib/scala/searcher/src/test/scala/org/enso/searcher/sql/SuggestionsRepoTest.scala
None,
None,
None,
Some(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

For completeness, would be probably good to have a test that checks non-static methods only

@mergify mergify bot merged commit 4c62dc9 into develop Mar 23, 2023
@mergify mergify bot deleted the wip/db/5874-from_rows-and-new-suggestions-in-cb branch March 23, 2023 15:02
Procrat added a commit that referenced this pull request Mar 27, 2023
@Procrat
Merge branch 'develop' into wip/procrat/node-action-bar-tooltips-5933
8b203e6 
* develop:
  Layout fixes (#6066)
  Use new Enso Hash Codes and Comparable (#6060)
  Search suggestions by static attribute (#6036)
  Use .node-version for pinning Node.js version (#6057)
  Fix code generation for suggestion preview (#6054)
  Implementation of EnsoGL predefined Rectangle shape. (#6033)
  Tidy up the public module level statics (#6032)
  Cursor aware Component Browser (#5770)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@farmaazon farmaazon farmaazon approved these changes

@hubertp hubertp hubertp approved these changes

@MichaelMauderer MichaelMauderer Awaiting requested review from MichaelMauderer

@wdanilo wdanilo Awaiting requested review from wdanilo

@mwu-tow mwu-tow Awaiting requested review from mwu-tow

@kazcw kazcw Awaiting requested review from kazcw kazcw is a code owner

Assignees

@4e6 4e6

Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

from_rows and new suggestions in CB while opening with the Table selected
4 participants
@4e6 @hubertp @farmaazon @JaroslavTulach