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

fix: handle cancellation of user-inputted delimiters #89

Closed smjonas closed 2 years ago

smjonas commented 2 years ago

Closes #85. This change also avoids an error when anything other than a table is returned from the pairs function.

kylechui commented 2 years ago

The only reason I'm hesitant on merging this is that it also forces the end-user to set up their own config using protected calls. I think if the pcall was abstracted into a different function and then provided to the user as an interface, that would make things easier/simpler. However, it can't be included in utils since utils requires config. I'm open to any suggestions you might have on how to organize the code.

smjonas commented 2 years ago

The only reason I'm hesitant on merging this is that it also forces the end-user to set up their own config using protected calls.

If a user does not protect their calls inside the pairs, an error should still be shown as before, no? My goal of this PR was to cover all cases where a call to vim.fn.input is already present (since these are default functions), not catch any other errors (except when something different than a table is returned).

kylechui commented 2 years ago

Yes that's correct, but I'm asking whether it would be easier for the user to call a nvim-surround-provided function (e.g. require("nvim-surround.config").get_input()) and have that protected call already "baked in", as opposed to needing to learn how to use pcalls themselves. I'm not entirely sure if we should expect the user to learn "more advanced" Lua in order to use/maintain this plugin.

smjonas commented 2 years ago

Ah, got you. Could that maybe be done in a separate PR since it adds a new feature instead of a fix? Could you tell me what I am still missing in this PR? :)

kylechui commented 2 years ago

Like I said before---I don't really know what "to expect" from the user base; it could be completely possible to just add a note about using pcalls whenever user inputs are used, since if user-inputs are not solicited this is a non-issue.

kylechui commented 2 years ago

Might be a nitpick but could you change your function prompt to get_input instead? I think it makes more sense with the naming scheme I've been using elsewhere in the project.

kylechui commented 2 years ago

Gimme one second while I learn how to leave comments and stuff on PRs

smjonas commented 2 years ago

Like I said before---I don't really know what "to expect" from the user base; it could be completely possible to just add a note about using pcalls whenever user inputs are used, since if user-inputs are not solicited this is a non-issue.

Sorry for being so stubborn on this, but we can't really control what users do inside their customized functions, right? So of course if an error occurs in their own function there will be issues. I don't think this is the responsibility of nvim-surround (neither to warn users nor to catch any errors).

Exposing a separate input function as you suggested (which will catch errors!) is something different.

kylechui commented 2 years ago

No worries! Discussions like these are very important to me to help me understand what the user-base wants, even if only a small portion.

With respect to you saying "it's not the responsibility of nvim-surround to warn users/catch errors", I would disagree. While I agree about not necessarily catching all errors (as you said, we can't control what users do in their functions), I think that this use-case where users are confused about cancelling inputs "not working" is something to be addressed. Even from a logistical standpoint, I see the cases where if nothing is mentioned about pcalls, then people are just going to create more issues regarding how "this is broken".

As of right now, I'm leaning towards just merging your PR and adding a message to the user (perhaps in the help docs section about using function-evaluated surrounds) that just conveys the message that pcalls should be used whenever user input is solicited.

smjonas commented 2 years ago

That sounds like a good plan! Though, I would keep the description even more general than just for user input (users should simply always handle errors themselves). The fact that vim.fn.ui or vim.ui do not handle keyboard interrupts "more gracefully" sounds more like an implementation detail to me.

kylechui commented 2 years ago

Sounds perfect, as soon as you fix my two requests I'll merge this into main, and I'll add the documentation.

smjonas commented 2 years ago

Sorry, could you clarify your two requests again? :smile: Was renaming prompt to get_input one of them? If so, I have already changed that.

kylechui commented 2 years ago

I don't know how to link to them, but I started a "code review" with proposed changes to the code; try scrolling up and/or checking the "files changed" tab?

smjonas commented 2 years ago

I think you need to click on "Finish your review" at the top right first :D It's also possible to add single review comments but everyone will be notified for each comment then which can become quite annoying.

kylechui commented 2 years ago

Sorry to keep prolonging this process, but can you update the README/help docs with the new default? I should really figure out some way to make it automatically update/just make them refer to config.lua.

smjonas commented 2 years ago

Just noticed that is says require("nvim-surround.utils").get_input() in the readme which does not exist yet. Should we just wait until it exists in a future PR? Otherwise I would have to copy the local get_input function which would be a bit unusual.

kylechui commented 2 years ago

Just noticed that is says require("nvim-surround.utils").get_input() in the readme which does not exist yet. Should we just wait until it exists in a future PR? Otherwise I would have to copy the local get_input function which would be a bit unusual.

This is from an older version of the code, please delete it.

kylechui commented 2 years ago

@smjonas Actually can you just change the details tag to a link that refers to to config.lua, e.g. The default configuration [can be found here](https://github.com/kylechui/nvim-surround/blob/main/lua/nvim-surround/config.lua).

smjonas commented 2 years ago

Done. Does that look good for you?

smjonas commented 2 years ago

Wait, seems like I need to rebase.

kylechui commented 2 years ago

Thanks again for the pull request, I really appreciate your time/effort :sparkling_heart:

smjonas commented 2 years ago

Thank you as well, great job on the plugin by the way! :partying_face:

smjonas commented 2 years ago

Unrelated to this PR but would you be interested in enabling GitHub actions for stylua and tests? I could create a PR for that tomorrow if you want.

kylechui commented 2 years ago

Yes, please! I'm quite new to using GitHub for actual collaboration with others (since all my past repos have been mainly private/just me committing), so if you could take the time to do that it would be much appreciated