Skip to content

ref: don't pass event around but bufnr, use autocmd event #116

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

Merged
merged 1 commit into from
Apr 17, 2025

Conversation

Kazy
Copy link
Contributor

@Kazy Kazy commented Apr 17, 2025

This doesn't impact anything feature wise nor does it fix any bug, but when implementing #115, I initially lost a bit of time trying to understand why the event from the DiagnosticChanged didn't have the diagnostics field. The main problem was that in each autocmd we're reading the event passed from LspAttach instead of the one from the triggered event itself.

This changes that and also only pass the buffer number around instead of the event to make things clearer. I've also moved the insertion of values into DISABLED_MODES outside of the LspAttach event to avoid duplicating the entries in it for nothing.

If you want to see a merged version of it with #115, you can check https://github.com/Kazy/tiny-inline-diagnostic.nvim/tree/integrated.

@rachartier
Copy link
Owner

Like the other PR, it's really great.

Now that you say it, it's true that passing around event is not a good practice to do.
If it can made futur PR easier to do, then it's the right call to merge your PR.

Thank you !

@rachartier
Copy link
Owner

Ah... there is a lots of conflicts. Can you maybe send a PR from your integrated branch?

@Kazy Kazy force-pushed the ref-au-event-bufnr branch from 825abf0 to 0e8e6b1 Compare April 17, 2025 21:57
@rachartier rachartier merged commit 8ea688b into rachartier:main Apr 17, 2025
1 check passed
@rachartier
Copy link
Owner

Thank you a lot !

@Kazy Kazy deleted the ref-au-event-bufnr branch April 17, 2025 22:01
@Kazy
Copy link
Contributor Author

Kazy commented Apr 17, 2025

I didn't get to even send my message to say that I had rebased the PR :p Thanks for the fast review and merge, appreciate !

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