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

Modernization Refactor (and bug fix) #20

Merged
merged 4 commits into from
Jun 19, 2024
Merged

Conversation

mehalter
Copy link
Contributor

@mehalter mehalter commented Apr 23, 2024

Hey! This is such a great plugin and I wanted to give a bit of a code refresh while also improving the API slightly by allowing specific buffers to be provided for the indentation detection. So there are a few goals of this PR:

  1. Being able to provide specific buffer numbers as context. The functions operate on the "current buffer" which works ~99% of the time but every once in a while a buffer may be opened in the background of the current buffer which can cause the detection to not happen. This adds the ability to provide a number as the context to set to a specific buffer even if it isn't the current. I also improved the auto commands to use this new feature and pass the context from the autocommand itself which resolves these cases where buffers are opened in the background!

  2. Use the more modern Lua APIs for the code. This removes all of the vimscript strings that were in the original codebase (for setting the user commands and autocmds). These have been around for a while and it is safe to adopt these. This makes things slightly faster (but we are juggling milliseconds). This also moves away from a couple APIs that are being deprecated in upcoming Neovim releases. Specifically vim.api.nvim_buf_set_option and replaces it with the simpler table notation

  3. Utilize the Lua Language Server annotations to document the API and make it easier to maintain down the line. This improves the type safety a bit and also provides LSP integration to the user if they use it. Now if the user does require("guess-indent").setup({ }) they will get auto completion within the table passed to the setup function!

Note

Also important to note, I made sure that this was implemented as non-breaking as possible. The current approach is non-breaking for the user facing command :GuessIndent but is breaking for the require("guess-indent").set_from_buffer arguments. From the docs it doesn't seem to be promoted for users to use this function so this seems like a pretty good approach

Definitely the main goal of the PR was to add the buffer number specification and improve the autocommands/user commands, but thought that maybe a bit of a refactor could help future-proof the plugin a bit as Neovim continues to grow and evolve without increasing burden on the plugin maintainer. Let me know if you have any comments or concerns! Thanks again for this great plugin, it continues to work perfectly!

@NMAC427
Copy link
Owner

NMAC427 commented Apr 25, 2024

Thank you! Overall, these changes look great. However, I'm not entirely sure if the arguments of the set_from_buffer function get handled properly. With your current implementation, calling :GuessIndent with a buffer number (or any kind of argument for that matter), it always gets treated as if it were called by an auto_cmd, meaning that all user output get's supressed. Previously this was only the case when calling :GuessIndent auto_cmd.
Do you think this the right behaviour? If a user goes out of their way to specify which buffer they want to be affected, I feel like they would probably also want to get feedback on what / if indentation got detected.

@mehalter
Copy link
Contributor Author

Thanks for this feedback! Yeah I thought about this as well. The main driving factor was to keep away from breaking changes to the API. The only way to avoid this would be to change the API to be better suited to being able to provide a buffer number. What do you think? Should I look at modifying the API in a way to better fit the ability to pass in the buffer number as well as specify the "auto_cmd" part?

@mehalter
Copy link
Contributor Author

@NMAC427 I just pushed a commit to resolve this, I separated out the bufnr and autocmd functionality for set_from_buffer. This is a breaking change for that function definition. I updated :GuessIndent to accept 2 arguments and provided details of that in comments in the code. The user facing command is not a breaking change with the extra parsing to make it hopefully not noticeable to the user. I looked at the docs and they don't seem to promote directly using the exposed Lua API so hopefully it won't affect too many users that the function signature for set_from_buffer has changed!

Let me know what you think or if you have any other ideas for this improvement. Thanks again for considering this!

@mehalter
Copy link
Contributor Author

mehalter commented May 6, 2024

Ok, I have been noodling about this quite a bit and I think I have come up with a good interface for the :GuessIndent command that satisfies all of the needs. The user can supply several arguments to control the indentation detection. They can specify: a specific buffer number, "context" (or "auto_cmd" for legacy support) to respect the buffer's context, and "silent" to silence the notification. Here are some examples to clarify:

:GuessIndent                   -- current buffer, no context, show notification
:GuessIndent context           -- current buffer, respect context, show notification
:GuessIndent auto_cmd          -- same as above, left in for legacy support
:GuessIndent context silent    -- current buffer, respect context, no notification
:GuessIndent silent            -- current buffer, no context, no notification
:GuessIndent 10                -- buffer 10, no context, show notification
:GuessIndent 10 context silent -- buffer 10, respect context, no notification

Based on the implementation, the arguments can be supplied in any order and only the first number is used as a buffer number.

Hopefully this deep dive and testing out different APIs is found to be useful! I really love this plugin and the approach it takes on detecting indentation and love the opportunity to help with the development 🤗

@Cretezy
Copy link

Cretezy commented May 10, 2024

I have been running into a bug where after I open a file, any other file opened with :e throws an error (yet still opens):

image

This branch fixed my problem, please take a look @NMAC427 :)

@mehalter
Copy link
Contributor Author

@NMAC427 Hey! Hope all is well, just wanted to check in and see if you have gotten any time to take a look at this PR now that it's been cleaned up. Thanks so much!

@NMAC427 NMAC427 merged commit 6c75506 into NMAC427:main Jun 19, 2024
3 checks passed
@NMAC427
Copy link
Owner

NMAC427 commented Jun 19, 2024

Sorry for not getting back to you sooner. Thank you for the great work!

@mehalter
Copy link
Contributor Author

No worries at all! Thanks so much for the consideration!

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.

3 participants