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

Feat: Grab comments above global symbols and show them on hover #34

Conversation

WilsonZiweiWang
Copy link
Contributor

This PR adds the comments above the symbols (Variables and functions) defined in the global scope in the hover definition.
Sample results:
Show all comments in the current file and included files
image
The comments are appended at the end
image

Caveat:
The results won't reflect right after modifications on the comments in the included files because it needs to run analyze() to update the values. Currently, it needs additional input to trigger it.

What I have in mind are these two:

  1. Somehow trigger the analyze() on the current file when the included files are modified.
  2. Maintain the references of the comments in the included files so that the current file always gets the updated comments.

Despite the caveat, the feature itself is ready for review.

@deribaucourt
Copy link
Member

Looks very promising! The idea of this feature is extending the hover documentation functionality to variables not covered by the poky doc, mostly from community layers. The goal should not be to display all comments. We have the Go to definition feature for navigating definitions and related comments.

A few comments:

  1. If a hover documentation from the Yocto docs are present, don't add comments. We already have the variable's doc from an official source.
  2. You can hence remove the "Comments" title
  3. Having multiple files comments is nice but a bit cumbersome. You should consider only keeping one with the logic we have discussed on Mattermost: .bbclass > .conf > .inc > .bb > .bbappend. If multiple files of the same extension provide comments, pick one at random. We have no way of determining the order yet.
  4. It's alright if analyze() is not called automatically unless a specific event happens in the current file. It can be one of: the user saves the file; the user modifies the file.

Copy link
Member

@deribaucourt deribaucourt left a comment

Choose a reason for hiding this comment

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

Code looks good

@WilsonZiweiWang
Copy link
Contributor Author

WilsonZiweiWang commented Dec 21, 2023

A few comments:

  1. If a hover documentation from the Yocto docs are present, don't add comments. We already have the variable's doc from an official source.
  2. You can hence remove the "Comments" title
  3. Having multiple files comments is nice but a bit cumbersome. You should consider only keeping one with the logic we have discussed on Mattermost: .bbclass > .conf > .inc > .bb > .bbappend. If multiple files of the same extension provide comments, pick one at random. We have no way of determining the order yet.
  4. It's alright if analyze() is not called automatically unless a specific event happens in the current file. It can be one of: the user saves the file; the user modifies the file.

1,2: Done.
3: Prioritizing comments done, but which .conf files should be checked?
4. I am working on another workaround which can technically skip the analyze(), and the current file will still get the updated results when the include/inherit files are modified

@WilsonZiweiWang WilsonZiweiWang force-pushed the Feat-10246-grab-comments-above-function-as-part-of-documentation branch from 6f5fd60 to 53a3550 Compare December 21, 2023 18:06
@WilsonZiweiWang WilsonZiweiWang force-pushed the Feat-10246-grab-comments-above-function-as-part-of-documentation branch from 53a3550 to 3d76325 Compare December 21, 2023 18:19
@WilsonZiweiWang WilsonZiweiWang marked this pull request as draft December 22, 2023 03:49
@WilsonZiweiWang WilsonZiweiWang force-pushed the Feat-10246-grab-comments-above-function-as-part-of-documentation branch from 3878293 to 627129e Compare December 22, 2023 04:43
@deribaucourt
Copy link
Member

Currently we don't bring .conf files to our recipes list so ignore them for now. Could become relevant when we have recipe-specific scan with the full include list.

@WilsonZiweiWang WilsonZiweiWang marked this pull request as ready for review January 3, 2024 16:13
@WilsonZiweiWang WilsonZiweiWang merged commit 1b8a61a into yoctoproject:staging Jan 3, 2024
1 check passed
@WilsonZiweiWang WilsonZiweiWang deleted the Feat-10246-grab-comments-above-function-as-part-of-documentation branch January 3, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants