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 go-to definition in metadata #4441

Merged
merged 5 commits into from
May 29, 2024

Conversation

cheerio-pixel
Copy link
Contributor

Reviving the work of razzmat, I have reworked his pull request to support this feature.

@github-actions github-actions bot added documentation client One or more of lsp-mode language clients labels Apr 26, 2024
@jcs090218
Copy link
Member

@razzmatazz WDYT? 🤔

Copy link
Collaborator

@razzmatazz razzmatazz left a comment

Choose a reason for hiding this comment

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

Approved with some suggestions to rename omnisharp-related functions to have omnisharp in the name.

clients/lsp-csharp.el Outdated Show resolved Hide resolved
clients/lsp-csharp.el Outdated Show resolved Hide resolved
clients/lsp-csharp.el Outdated Show resolved Hide resolved
clients/lsp-csharp.el Outdated Show resolved Hide resolved
@razzmatazz
Copy link
Collaborator

@razzmatazz WDYT? 🤔

Reviewed, thank you! Approved 👍🏻 , with a suggestion to rename omnisharp-related fns to have omnisharp in the name

@jcs090218
Copy link
Member

@cheerio-pixel Friendly ping! :)

Now if we try to find the definition of a symbol that is defined
externally, it will try to load a stripped-down version of the file where
is defined and show it in a temporal file.
@cheerio-pixel
Copy link
Contributor Author

cheerio-pixel commented May 29, 2024

@cheerio-pixel Friendly ping! :)

Sorry, completly forgot everything about this. Thanks for the ping.

I renamed functionallity only used by omnisharp to be prefixed with
lsp-csharp--omnisharp-

I'm a little bit torn with lsp-csharp--path->qualified-name being prefixed
like that, but only omnisharp is using it.
@cheerio-pixel
Copy link
Contributor Author

cheerio-pixel commented May 29, 2024

@jcs090218 Finished applying the suggestions. Sorry for messing up with the historial. I didn't realize that rebase modifies the authoring information.

@jcs090218
Copy link
Member

Can you fix these compile warnings? Thanks! :)

In lsp-csharp--omnisharp-metadata-uri-handler:
lsp-csharp.el:358:2: Warning: docstring has wrong usage of unescaped single quotes (use \= or different quoting)

In lsp-csharp--omnisharp-uri->path-fn:
lsp-csharp.el:394:2: Warning: docstring wider than 80 characters

clients/lsp-csharp.el Outdated Show resolved Hide resolved
@cheerio-pixel
Copy link
Contributor Author

@jcs090218 Anything more?

Copy link
Member

@jcs090218 jcs090218 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you! :)

@jcs090218 jcs090218 merged commit 750043c into emacs-lsp:master May 29, 2024
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client One or more of lsp-mode language clients documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants