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

upstream to :help lua-guide #5

Open
justinmk opened this issue May 17, 2024 · 13 comments
Open

upstream to :help lua-guide #5

justinmk opened this issue May 17, 2024 · 13 comments

Comments

@justinmk
Copy link

justinmk commented May 17, 2024

Your notes here are quite good. Would you be willing to upstream (most of) it to :help lua-guide :help lua-plugin (in a new lua-plugin.txt help file)? This would allow us to close neovim/neovim#22366

Some of the controversial parts could be skipped for now, but 85% of what you have would give a great starting point for :help lua-plugin.

@mrcjkb
Copy link
Member

mrcjkb commented May 17, 2024

Thanks for the nice words 😄

I'd be more than happy to.

@mrcjkb
Copy link
Member

mrcjkb commented May 24, 2024

@justinmk would you like me to open a PR to Neovim?
If so, let's clarify here (or in neovim/neovim#22366) which points to include first (and how best to structure them).

@justinmk
Copy link
Author

justinmk commented May 25, 2024

yes! here's a rough idea:

Note: historically the discussion about setup() has been controversial, but I think you wording is good, because you leave the door open for judgement.

include these

  • ## Type safety (all)
  • ## User Commands (all)
  • ## Keymaps (all)
  • ## Initialization (all)
  • ## Configuration
    • section: ### DO ...validate configs.
  • ## Troubleshooting (all)
  • ## Versioning and releases (all)
  • ## :notebook: Documentation (all)

skip these for now (to unblock the PR / avoid controversy)

  • ## Configuration: DO ...use [LuaCATS annotations]
    • this one is pretty granular and skipping it will save time for now.
  • ## Testing
    • Too big of a topic to cover in lua-guide. Eventually we should have a concise answer with sample code, that doesn't require lots of setup.
  • ## Integrating with other plugins
    • Agreed, but we don't have enough to say about this yet.

@justinmk

This comment was marked as duplicate.

@echasnovski
Copy link

## User Commands (all)

I don't think suggesting to prefer one umbrella command to several targeted commands is an enough universal advice to be included in core. To me this is a more question of taste. "Pollute the command namespace" is obviously a bad thing if formulated this way, while "provide more narrowly scoped commands" is not .

## Keymaps (all)

I don't think suggesting to create a <Plug> for users to map is a good design for "modern Neovim". If anything, suggesting to export a Lua function which can be used in both "<Cmd>lua xxx()<CR>" and function() xxx() end right hand side is a more straightforward approach with less overhead for plugin author. I have read the "benefits of <Plug> mappings over exposing a lua function", yet most of them are not (per se) benefits of preferring <Plug> over Lua functions.

And "If a user has to call a setup function to set keymaps, it will cause an error if your plugin is not installed or disabled." is not necessarily a bad side effect.

## Initialization (all)

Do not agree at all that this is worded as to "leave the door open for judgement". The "Automatically initialize your plugin (smartly), with minimal impact on startup time." might sound good on paper, yet:

  • "Minimal impact" is still an impact which would make extreme startup optimization not possible (or again needing some hacky plugin manager). As it is a big part of what makes Neovim fun to use for many people, I'd be careful to not make it harder.
  • Initialization itself might need some configuration (tweaking which and how autocommands / user commands / highlight groups / mappings are created).

Having setup() be a single entry point for plugin to create any side effects feels like the easiest to grasp concept for both new users and new plugin authors. Yes, it goes against "installed plugin should work right away" mentality in favor of "installing plugin is just downloading code, enable it with setup()" approach.

## Versioning and releases (all)

"Don't use 0ver" suggestion is a bit ironic to be included in Neovim itself and is not a universally good advice. Many successful projects work just fine with 0ver.

@mrcjkb
Copy link
Member

mrcjkb commented May 25, 2024

@echasnovski thanks for the input. It's definitely great to get
feedback from someone with strongly differing views than me,
as it sheds some more light on what might be considered too controversial
for core 😄

I don't think suggesting to prefer one umbrella command to several targeted commands is an enough universal advice to be included in core. To me this is a more question of taste.

Just to be clear, I don't advocate for a single command with subcommands per plugin.
For example, in rustaceanvim, I provide more than one command.

  • RustLsp {subcmd} for interacting with the LSP client
  • RustAnalyzer [start|stop|restart].
  • ...

It's definitely not universal advice, and depends on the use case.
I do think it's worth recommending to think carefully about whether one
might be polluting the command namespace.

yet most of them are not (per se) benefits of preferring <Plug> over Lua functions.

Could you please elaborate on this?

Being able to enforce things like expr = true (and thus preventing people from
opening issues because they forgot to do so),
or exposing different functionality for different modes, are very clear benefits.

Sure, these are not always needed, which is why I also have a DO section
for just exposing a Lua API.
Again, this is something that depends on the use case.

with less overhead for plugin author

A bit of overhead can be a massive time saver if it results in less time wasted
supporting people who have issues with your plugin.

And "If a user has to call a setup function to set keymaps, it will cause an error if your plugin is not installed or disabled." is not necessarily a bad side effect.

If it weren't forced on the user, I would agree.

"Minimal impact" is still an impact which would make extreme startup optimization not possible (or again needing some hacky plugin manager). As it is a big part of what makes Neovim fun to use for many people, I'd be careful to not make it harder.

This seems like a slippery slope argument. I don't see how these steps are necessarily connected.
In fact, your next point, 'Initialization itself might need some configuration,'
suggests a way to make 'extreme startup optimization' possible.
And a setup function is not the only thing that makes configuring initialization possible.

Having setup() be a single entry point for plugin to create any side effects feels like the easiest to grasp concept for [...] new users

That perspective is highly subjective and likely influenced by the status quo.
Not everyone prefers to be forced into a single entry point.
For instance, some may prefer separate files for keymaps, highlight groups,
and other configurations.

In fact, the only 'complaint' I've ever heard about one of my plugins not providing a setup function
would have been satisfied if I had provided a setup function that does absolutely nothing
but set a vim.g. table.

for [...] new plugin authors

Bad practices tend to be easier to grasp.

"Don't use 0ver" suggestion is a bit ironic to be included in Neovim itself

I fully agree with this.

Although personally, I do believe "SemVer to properly communicate bug fixes, new features, and breaking changes." is the best approach, and it would be beneficial if it were consistent across the ecosystem.

@echasnovski
Copy link

Just to be clear, I don't advocate for a single command with subcommands per plugin. For example, in rustaceanvim, I provide more than one command.

* `RustLsp {subcmd}` for interacting with the LSP client

* `RustAnalyzer [start|stop|restart]`.

* ...

This looks like violating the advice as it is written, because it should instead be Rust lsp {subcmd} and :Rust analyzer [start|stop|restart]. If it is not treated as "one command per plugin" or at least "one command per exported module", then it does not have much sense as "Don't do that, do this instead" type of recommendation. As an advice that both types of commands are possible - sure, that's great.

Could you please elaborate on this?

Being able to enforce things like expr = true (and thus preventing people from opening issues because they forgot to do so), or exposing different functionality for different modes, are very clear benefits.

Forcing mapping options is the sole benefit here, yes.

"Expose functionality for specific modes" and "Expose different behaviour for different modes with a single <Plug> mapping." are not specific to <Plug> approach and only lead to more code without clear benefit. Both of them still require code to handle different modes, which can be directly exposed to the user. So "one <Plug> for different modes" can easily be "one Lua function for different modes".

And "If a user has to call a setup function to set keymaps, it will cause an error if your plugin is not installed or disabled." is not necessarily a bad side effect.

If it weren't forced on the user, I would agree.

Suggesting instead to force on the user to create mappings themselves does not look like a clear advantage in this aspect.

That perspective is highly subjective and likely influenced by the status quo.
Not everyone prefers to be forced into a single entry point.
For instance, some may prefer separate files for keymaps, highlight groups,
and other configurations.

As not everyone prefers things to be forcefully executed. Separate files for keymaps, highlight groups, and other configurations are certainly possible with "single entry point setup()" approach.

for [...] new plugin authors

Bad practices tend to be easier to grasp.

As is bad advice.


If anything, my main issues with most of the suggestions here can probably be summarized as:

  • Lua is a fully featured programming language with first class support in Neovim and this should be utilized at maximum capacity. Officially recommending Vimscript-esque approaches like <Plug> and g: is a serious step backwards.
  • Flexibility of Neovim itself and how it is configured has proved to be a vital part of its popularity among users. Thus official suggestions should either advocate for maximum flexibility approach (to me this feels like a current single setup() approach) or only list approaches without advocating for any particular one.

@justinmk
Copy link
Author

justinmk commented May 25, 2024

Officially recommending Vimscript-esque approaches like <Plug> and g: is a serious step backwards.

<Plug> is a mapping feature, it's separate from vimscript. Neither is a "serious step backwards".

I don't want this to be blocked on nitpicks or concerns about style. Plugin authors need a place to start, and in all of the discussion above I don't see any concrete "bad advice". Style preferences are not "bad advice", and plugin authors can choose to diverge if they want. But we need to give a starting point.

@echasnovski
Copy link

Style preferences are not "bad advice", and plugin authors can choose to diverge if they want. But we need to give a starting point.

Having "don't do this approach" with this exact wording inside core help is a bit more than "it is just a style preferences".

Modifying the text to contain only "do"s and mentioning other approaches without shedding bad light will go a long way.

@mrcjkb
Copy link
Member

mrcjkb commented May 25, 2024

for [...] new plugin authors

Bad practices tend to be easier to grasp.

As is bad advice.

@echasnovski In case of misunderstanding: My response aimed to clarify that being easy to grasp does not necessarily equate to "good practice."

Your response did not address the substance of my point and seemed more focused
on confrontation rather than constructive discussion.

This approach is counterproductive and detracts from a respectful and meaningful exchange.
I would like to point you to our Code of conduct.

If we cannot keep this discussion civil and focused on the topic,
I will close this issue as "not planned" due to concerns about negative interactions
and will block further communication with you.
The Neovim core team will still have my consent to use this document freely.

To address your other concerns:

This looks like violating the advice as it is written

This seems like a misinterpretation of the advice on your part.
Perhaps it would help if I added a tip box to the section to clarify?

"one <Plug> for different modes" can easily be "one Lua function for different modes"

Yes, that is possible, but it would involve adding side effects
and increasing the complexity of the Lua function.

Suggesting instead to force on the user to create mappings themselves

We're discussing <Plug> mappings versus Lua functions,
not forcing users to create mappings themselves (this is a straw man).
If you're shifting to the question of why I don't advocate for keymap DSLs,
my reasoning is outlined in the document.

As not everyone prefers things to be forcefully executed.

This is another straw man. I never advocated for things to be forcefully executed.
As I mentioned in my previous response:

In fact, your next point, 'Initialization itself might need some configuration,'
suggests a way to make 'extreme startup optimization' possible.
And a setup function is not the only thing that makes configuring initialization possible.

Separate files for keymaps, highlight groups, and other configurations are certainly possible
with "single entry point setup()" approach.

Some plugin managers provide abstractions for doing this.
However, I imagine doing this manually could be quite tedious. But maybe I'm mistaken?

Lua is a fully featured programming language with first class support in Neovim

Vimscript, too, has first class support in Neovim.

Officially recommending Vimscript-esque approaches like <Plug> and g: is a serious step backwards.

My document does not make any recommendations to use vim.g
(I do in my blog post, which is more opinionated).
I simply state that this is my preference and that there is no real downside to using it (I'm aware of the theoretical downsides).
To the contrary, recommending against Vimscript compatibility without good reason
would be a step in the wrong direction.

official suggestions should either advocate for maximum flexibility approach (to me this feels like a current single setup() approach)

I have yet to be presented with evidence that a single setup approach is more flexible
than (configurable) smart initialization.

Modifying the text to contain only "do"s and mentioning other approaches without shedding bad light will go a long way.

I agree that some of the DON'Ts could probably be excluded from core.
Given how strongly you feel about setup, even if I strongly disagree with you,
I do feel that perhaps the official help docs shouldn't make any recommendations against it.

But some of the DON'Ts (e.g. don't create keymaps automatically) are quite uncontroversial,
and not immediately obvious to newcomers.

@echasnovski
Copy link

for [...] new plugin authors

Bad practices tend to be easier to grasp.

As is bad advice.

@echasnovski In case of misunderstanding: My response aimed to clarify that being easy to grasp does not necessarily equate to "good practice."

Your response did not address the substance of my point and seemed more focused on confrontation rather than constructive discussion.

This approach is counterproductive and detracts from a respectful and meaningful exchange. I would like to point you to our Code of conduct.

If we cannot keep this discussion civil and focused on the topic, I will close this issue as "not planned" due to concerns about negative interactions and will block further communication with you. The Neovim core team will still have my consent to use this document freely.

@mrcjk, in case of misunderstanding: no confrontation was intended. My response aimed to clarify that being easy to grasp should be one of the main goals of built-in help for creating Lua plugins, while avoiding bad advice at the same time.

@wsdjeg
Copy link

wsdjeg commented Jul 29, 2024

agree with @echasnovski , I do not think the sub command section and version release section should be merged into neovim main doc.

there should be a guide to avoid user to write wrong code, for example, create autocmd without augroup. It maybe broken when other plugin override the autocmd.

All roads lead to Rome

@mrcjkb
Copy link
Member

mrcjkb commented Jul 29, 2024

@wsdjeg see my response to your other comment in the neovim PR.

there should be a guide to avoid user to write wrong code, for example, create autocmd without augroup. It maybe broken when other plugin override the autocmd.

Agreed. It's that a common footgun? Although maybe that should be (or is?) included in the respective autocommand docs?
Then a very short note on it in this guide could link to that.

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

No branches or pull requests

4 participants