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: Implement find references for #hashtags #258

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

TheBlob42
Copy link
Contributor

@TheBlob42 TheBlob42 commented Oct 27, 2023

fixes #194

Followed the guidance by @artempyanykh in the ticket. Thanks for this 👍 Without it I would have needed way more time to find myself around the project.

Tested with Neovim and it works as expected:

Peek 2023-10-27 22-54

I just want to note that I have never coded in F# before and I did not really read into all its specialties and so on. Therefore please point out parts that could be improved or be written in a more F# way for me to learn.

Copy link
Owner

@artempyanykh artempyanykh left a comment

Choose a reason for hiding this comment

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

@TheBlob42 thanks for submitting a pull request! It's pretty close but needs a few tweaks. Let me know if you want to give it another round based on the comments.

Marksman/Refs.fs Outdated Show resolved Hide resolved
Marksman/Refs.fs Outdated Show resolved Hide resolved
Marksman/Refs.fs Outdated Show resolved Hide resolved
Marksman/Refs.fs Show resolved Hide resolved
@TheBlob42
Copy link
Contributor Author

Hey @artempyanykh thanks for the feedback 👍

I will have a look at them throughout the next days and either ask questions or adapt my PR for it

@TheBlob42
Copy link
Contributor Author

TheBlob42 commented Oct 29, 2023

I have adopted the PR according to your comments or at least how I understood them and to my best understanding. The project builds but the find "references functionality" does not work anymore for me. @artempyanykh maybe you could have a look into the last commit I made with the adaptions to see if I have misunderstood some of your points or made some other mistake that explains this

I have the feeling that I might have misunderstood some of the points made 🤔

Copy link
Owner

@artempyanykh artempyanykh left a comment

Choose a reason for hiding this comment

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

Thanks for updating the PR. There are just a couple small changes that are required to make this work.

Ideally, it'd be great to add a test for tag references resolution under RefsTest but up to you.

Marksman/Refs.fs Outdated Show resolved Hide resolved
Marksman/Refs.fs Outdated Show resolved Hide resolved
@artempyanykh artempyanykh added the enhancement New feature or request label Oct 29, 2023
@TheBlob42
Copy link
Contributor Author

I have adopted the code and it is working again.
I have added two simple tests for tag references based on the ones that were present for other elements in RefsTest.fs

Let me know if you see any further issues or things to improve 🙂

@artempyanykh
Copy link
Owner

@TheBlob42 thanks for adding the tests! One last thing is please run make fmt to reformat the code, otherwise CI is unhappy.

@artempyanykh artempyanykh merged commit 7f3b471 into artempyanykh:main Oct 30, 2023
3 checks passed
@artempyanykh
Copy link
Owner

All looks good! Thanks for contributing @TheBlob42!

@TheBlob42
Copy link
Contributor Author

Thanks you very much for all the guidance and patience 🙂 ❤️

@petobens
Copy link

Hi @artempyanykh is it possible to show tags as LSP document symbols? Currently we can use references and autocompletion for hastags but cannot use lsp document symbol method to list tags (since it only lists headers).

@artempyanykh
Copy link
Owner

@petobens it's possible, yes. Could you file a separate issue for this?

@petobens
Copy link

Sure. Done in #261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement 'find references' request for #hashtags
3 participants