-
Notifications
You must be signed in to change notification settings - Fork 39
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
Conversation
There was a problem hiding this 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
updated. |
Thank you for updating the PR body with the context regarding the sets. The set for the |
confirmed, |
Apart from the style change and the PR title, this PR is good to go. |
symToSuggest
restrict conditions to IdeCmd
symToSuggest
restrict conditions to IdeCmdsymToSuggest
restrict conditions to IdeCmd
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
symToSuggest
restrict conditions to IdeCmd
symToSuggest
limit data per IdeCmd
There was a problem hiding this 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.
@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>
/merge I've updated the PR message according to the recent changes. |
Summary
suggest.symToSuggest
now restrict data fetched based on theIdeCmd
given. Meaning some
Suggest
fields are only filled in/used givenspecific
IdeCmd
(s), avoiding unnecessary processing.Details
contextFits
,globalUsages
,localUsages
fields whenIdeCmd
in{ideSug, ideCon}
. they're used incmpSuggestions
->produceOutput
.doc
field whenIdeCmd
is notideHighlight
.ideHighlight
results don't render the documentation comment, see
$(Suggest)