search

kylechui / nvim-surround

Add/change/delete surrounding delimiter pairs with ease. Written with :heart: in Lua.
MIT License
3.18k stars 62 forks source link

`utils.get_input` silently removed #94

Closed 0xAdk closed 2 years ago

0xAdk commented 2 years ago

Checklist

Describe the bug utils.get_input got removed without anything being posted to breaking changes or the docs being updated

kylechui commented 2 years ago

Shoot, forgot to remove it from the docs. This was removed before Breaking Changes was created (I think), which is why it wasn't posted there. I'll add it in there right now though.

kylechui commented 2 years ago

@0xAdk I've added it to both the Breaking Changes issue, as well as updated the documentation to the latest version. Thanks for pointing this out, and feel free to re-open this issue if there's anything else I missed!

0xAdk commented 2 years ago

feel free to re-open this issue if there's anything else I missed!

no clue how I would do that, but you did miss two places :p https://github.com/kylechui/nvim-surround/blob/9e5c9ec48d0c44237d7bdcf7a030355524d56dba/doc/nvim-surround.txt#L267 https://github.com/kylechui/nvim-surround/blob/9e5c9ec48d0c44237d7bdcf7a030355524d56dba/doc/nvim-surround.txt#L282

btw while I have your attention would the plugin choosing to use vim.fn.input limit customization / using custom ui(s)? I was under the impression that utils.get_input was used so it could be replaced with a different interface if the user wanted

kylechui commented 2 years ago

Gah! Just fixed those two as well. Originally the plan was to have it so that vim.ui.input() could be used instead to allow for different interfaces, but it's an async function which causes all sorts of back-end problems. I really did try to use it, see #24, but was unsuccessful. I then later removed utils.get_input() because I was rearranging code and I wanted utils to be able to require config, but you can't have both files requiring each other simultaneously.

Also about re-opening issues, perhaps that's something that's only available on my end? Unsure :P

smjonas commented 2 years ago

I think reintroducing the utils.get_input() function that users can call and optionally overwrite seems quite useful (see also the discussion in #89). About the circular dependency issue: can't you just pass the config table to the function in utils?

kylechui commented 2 years ago

@smjonas I agree that it would be useful, but at the moment the code has:

Could you be a bit more specific as how to refactor the code to circumvent this? The other option that could work is to have config.get_input() instead of utils.get_input(), but that seems sub-ideal IMO

smjonas commented 2 years ago

Could you be a bit more specific as how to refactor the code to circumvent this? The other option that could work is to have config.get_input() instead of utils.get_input(), but that seems sub-ideal IMO

I meant that you could simply pass opts as an additional argument to functions like get_alias() which would make the dependency on the config module redundant.

kylechui commented 2 years ago

I see; IMO it makes the most sense to have all functions refer to config in order to get information about how the user has configured nvim-surround, and as you said earlier, leave issues of error handling to the user. I think theoretically input could be queried in other ways, e.g. functional surrounds only care that they receive their table in a synchronous matter; it just so happens to be that vim.fn.input() is the simplest way to do so.

kylechui commented 2 years ago

@smjonas Now that I think about it more, I don't think I'm that opposed to having a config.get_input(), but I think I need a strong(er) reason/some justification to proceed this way, to avoid pushing more Breaking Changes.

smjonas commented 2 years ago

I actually think utils.get_input() would make more sense. This is how I imagined it:

Instead, as an example, change:

M.get_delimiters = function(char, args)

to

M.get_delimiters = function(delimiters, char, args)

and call it from init.lua like this: local delimiters = utils.get_delimiters(config.get_opts().delimiters, ins_char, delim_args). This should work in a similar way for utils.get_input(). Would that work for you?

Also regarding breaking changes: maybe you could support semantic versioning for your plugin? This way, users can pin their plugin version to a specific major version like version 1 if they want to. This allows them to deal with breaking changes in their plugins when they choose to instead of when they update their plugins. For each breaking change, you would simply increase the major version. I know there is kind of this hesitation to increase major versions quickly but that's how semver works ;)

Packer supports this via tags:

use {
  "kylechui/nvim-surround",
  tag = "1.*",
}
kylechui commented 2 years ago

While this seems like it would work/make sense, I guess I'm still not completely sold on actually needing this to begin with, especially with the increased complexity of using utils over config. The main point is that most/all files will need to access the user-defined options at some point, and using config.get_opts() seems like a natural way to do so, instead of passing tables back and forth as parameters to a function.

smjonas commented 2 years ago

That's fine, I just thought this might be a way to circumvent the circular dependency issue. There are definitely other solutions as well :)

kylechui commented 2 years ago

@smjonas I think I'm going to end up creating config.get_input(), along with other functions like config.get_selection() to simplify the user configuration process. TL;DR allowing users to use functions to determine what to delete, etc. is a bit of a pain, so I think exposing a configuration API is a better idea.