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

Code action constantly suggests 'Update the Table of Contents', even when it is updated #52

Closed
David-Else opened this issue Aug 23, 2022 · 12 comments · Fixed by #58
Closed
Labels
bug Something isn't working

Comments

@David-Else
Copy link

David-Else commented Aug 23, 2022

Cool language server! :)

With marksman 2022-08-19 on Linux and Neovim I create a table of contents. I have nvim-lightbulb installed. Now the lightbulb is constantly lit (wherever my cursor is) telling me there is a code action to 'Update the Table of Contents', even when non of the headings have changed.

Is it possible to only announce this code action when a relevant item has changed? Thanks.

@artempyanykh
Copy link
Owner

@David-Else yeah, we definitely should do it! @keynmol do you think you'll get to #22 soonish? If not, I can take a stab at this issue myself.

@artempyanykh artempyanykh added the bug Something isn't working label Aug 24, 2022
@keynmol
Copy link
Contributor

keynmol commented Aug 25, 2022

Once #22 is merged and no further regressions are found (the whole damn thing is a regression), I can take a look at this
I agree in principle that code action shouldn't be offered if both statements are true:

a) it was detected in the workspace
b) the text and in-memory TOC result are the same

It will be a bit harder than I initially assumed because on first opening of the document one will need to parse and reconstruct the TOC, and do that on every document change (because nothing stops you from manually modifying the TOC)

@David-Else
Copy link
Author

David-Else commented Aug 25, 2022

So it will auto-update the TOC on save? That would be better, then there is no need for the update TOC code action. Nobody wants a wrong TOC, and having to remember to update it all the time, auto-update is a much better solution :)

@artempyanykh
Copy link
Owner

So it will auto-update the TOC on save?

That's a good point. I guess we could hook into willSaveWaitUntil request to do the necessary TOC update on save. But I think we'll still need a standalone code action too.

@keynmol
Copy link
Contributor

keynmol commented Aug 25, 2022

Yeah, it can be a configurable thing - I'm about 50/50 on automatically updated TOC, as I prefer to be in control of edits sent from the LSP, but also I like the convenience.

@artempyanykh
Copy link
Owner

@keynmol you gotta let go and submit to the machines man 😉

Seriously tho, I'm fine with a config in this case but we need to keep in mind that every config option adds more variability to the server's behavior, hence less testing and eyeballing for a particular set of configurations.

@artempyanykh
Copy link
Owner

The original problem with constantly suggesting to "Update TOC" is resolved now. Updating TOC "on save" feels like a separate feature. @David-Else feel free to open an issue for this if it's something you care about.

@David-Else
Copy link
Author

Cheers! I will test it when it gets released in a new version. You can close the issue, or I will after I test the fix.

@artempyanykh
Copy link
Owner

artempyanykh commented Aug 27, 2022

@David-Else
Copy link
Author

David-Else commented Aug 27, 2022

Great work, thanks! Unfortunately I do have a small problem, not sure the best solution. If Marksman creates a TOC, for example:

<!--toc:start-->
- [health](#health)
- [diy](#diy)
- [cleaning](#cleaning)
<!--toc:end-->

All is well, no code action spam... but when I format with Prettier using default settings it inserts a space at the top:

<!--toc:start-->

- [health](#health)
- [diy](#diy)
- [cleaning](#cleaning)
<!--toc:end-->

Now marksman sees it as a change and starts the update spam.

You can see the problem live here: https://prettier.io/playground/#N4Igxg9gdgLgprEAuEAeAhAWkzCYkDOMAhgE4zYB8AOlJgAQDaAFnMQDYzMC6AFAMSsOXAJS0GjACYBLAJ59+M2WLpMw7NlGlQA5gvWbtOlRmy58CSVVq1+9IZ2Y2odpc7sHiW3SAA0ICAAHGGloAmRQMlIIAHcABTIEcJQOGOJZcP8AI1JiMABrOBgAZWIAWzgAGW04ZAAzDgI4bNyCouLAvKNkGFIAV2aQODKsuElJMcqvHT7iHTgAMQhSMuIYEJ8Uvtw-EGYYMvYAdWZpeAJOsDhipLPpADcz2WRwAkyQbSbyONydVfrGoMAFYEAAexSMGgAin0IPAAewmv5OqQvi9VqR8pJYlBdoFSNoYEdpJIuMgABwABmR0SaR1ygRe+LgX3utX8AEdYfAfkFkiBiARMFA4GMxrtSHAudJJT85v8kA1EYMmmVpD1+irIXAYXDaorAf4SFliaTmMgAExG3LSdhGADCEDKCqGBAArLs+k0ACrELLJJVIkD3AYASSgE1gxTABOCAEEI8UYLINAimgBfdNAA

I have tried adding <!-- prettier-ignore --> at the top of the file above the TOC but it does nothing.

As I have format on save enabled this is a real problem. I hate to ask, but any chance it could be addressed? As Prettier is the main tool used by many to format markdown, maybe the space could be added when the TOC is created?

It would be best formatted with spaces on either side, as that is what Prettier will do if you create many spaces before toc:end (and I think it looks the best):

<!--toc:start-->

- [health](#health)
- [diy](#diy)
- [cleaning](#cleaning)

<!--toc:end-->

@keynmol
Copy link
Contributor

keynmol commented Aug 27, 2022

I had no idea there are formatters for markdown :)

This seems to be a very strange formatting on Prettier side - do you know what is it trying to do? I.e. what is the particular benefit of producing this newline after comment but not after the end of block of links..

Also, I just found this issue: prettier/prettier#10128 which seems to me that this behaviour of Prettier causes issues in other extensions as well.

Both adding newlines and preserving them when detecting TOC is probably not a massive amount of work, I just feel it will be done for the wrong reasons (circumventing a 3rd party plugin behaviour which is seen as a problem by others)

@artempyanykh
Copy link
Owner

Let's fork this into a separate issue #60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants