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

suggest: symToSuggest limit data per IdeCmd #908

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

bung87
Copy link
Contributor

@bung87 bung87 commented Sep 16, 2023

Summary

suggest.symToSuggest now restrict data fetched based on the IdeCmd
given. Meaning some Suggest fields are only filled in/used given
specific IdeCmd(s), avoiding unnecessary processing.

Details

  • Fill contextFits,globalUsages,localUsages fields when IdeCmd in
    {ideSug, ideCon}. they're used in cmpSuggestions -> produceOutput.
  • Fill doc field when IdeCmd is not ideHighlight. ideHighlight
    results don't render the documentation comment, see $(Suggest)

@bung87 bung87 changed the title refactor symToSuggest,restrict conditions to IdeCmd tool: suggest refactor symToSuggest, restrict conditions to IdeCmd Sep 16, 2023
@haxscramper haxscramper added this to the Tooling milestone Sep 16, 2023
Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

I think this change is okay, less processing is certainly nice, but I dislike that the data type (Suggest) doesn't reflect that not all fields are valid for each section value.

Eventually, Suggest should either be turned into a variant object or replaced with something completely different, but for now, could you document on the fields for which section the fields apply?


Regarding the PR message and title:

  • please try to keep the PR titles short. They're used as the commit title, and should not be much longer than 50 characters
  • the lines in the body must be no longer than 72 characters
  • please document where you got the information of what the sets should be from. Skimming over the symToSuggest usage sites, ideChk seems to never be passed to it

compiler/tools/suggest.nim Outdated Show resolved Hide resolved
@bung87
Copy link
Contributor Author

bung87 commented Sep 17, 2023

updated.

@zerbina
Copy link
Collaborator

zerbina commented Sep 17, 2023

Thank you for updating the PR body with the context regarding the sets.

The set for the doc field including ideChk is still a bit confusing, given that symToSuggest is never invoked with ideChk. However, since the whole suggestion reporting is going to be reworked anyway, I think that's fine for now.

@bung87
Copy link
Contributor Author

bung87 commented Sep 17, 2023

confirmed, ideChk doesn't goes to symToSuggest, structuredReportHook take the responsibility that accept Report, now leave it as is, as symToSuggest accept all commands and Suggest is not object variant, so take care all kinds of command is fine. I haven't trigger diagnostics so far, I'll revisit the code handle ideChk.

@bung87 bung87 requested a review from zerbina September 19, 2023 16:55
@zerbina
Copy link
Collaborator

zerbina commented Sep 19, 2023

Apart from the style change and the PR title, this PR is good to go.

@bung87 bung87 changed the title tool: suggest refactor symToSuggest, restrict conditions to IdeCmd tool: suggest symToSuggest restrict conditions to IdeCmd Sep 19, 2023
@bung87 bung87 changed the title tool: suggest symToSuggest restrict conditions to IdeCmd tool: suggest symToSuggest restrict conditions to IdeCmd Sep 20, 2023
@bung87 bung87 changed the title tool: suggest symToSuggest restrict conditions to IdeCmd tool: suggest symToSuggest restrict conditions to IdeCmd Sep 20, 2023
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
@saem saem changed the title tool: suggest symToSuggest restrict conditions to IdeCmd suggest: symToSuggest limit data per IdeCmd Sep 20, 2023
@saem saem added tool Improvements to non-compiler tooling simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Sep 20, 2023
Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

Sorry for not having noticed this earlier.

While rewording the PR message and looking for a more general reason for the doc field change, I noticed that the ideMod section is not included in the set, which is a breaking change (see the $ operator for Suggest).

I know that the custom protocol is going to be removed, but until it is, nimsuggest has to continue working.

compiler/tools/suggest.nim Outdated Show resolved Hide resolved
@saem
Copy link
Collaborator

saem commented Sep 20, 2023

@zerbina FYI, I just reworded the PR message, nothing major but hopefully I didn't overwrite your changes. 🤞🏽

Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
@zerbina
Copy link
Collaborator

zerbina commented Sep 21, 2023

/merge

I've updated the PR message according to the recent changes.

@github-actions
Copy link

Merge requested by: @zerbina

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

part of #879

@chore-runner chore-runner bot added this pull request to the merge queue Sep 21, 2023
Merged via the queue into nim-works:devel with commit 8beb528 Sep 21, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
simplification Removal of the old, unused, unnecessary or un/under-specified language features. tool Improvements to non-compiler tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants