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

Fixed problems with shellslash #363

Closed
wants to merge 5 commits into from

Conversation

andreadev-it
Copy link

These changes should fix most of the issue about using shellslash on windows. However, maybe it's better to wait for this PR on plenary to be merged before accepting this. Without the changes on plenary, many things will just not work anyway.

andreadev-it and others added 3 commits May 27, 2022 20:24
This should fix some problems that I found on windows when using the `shellslash` option to use forward slashes instead of backslashes.
@andreadev-it
Copy link
Author

Found a problem: on linux the shellslash option is not disabled, it doesn't exists at all, and the code breaks. I'm making a change on that regard (using pcall to handle the eventual error), but I'm not sure how to update the PR.

@andreadev-it
Copy link
Author

Tested on my linux laptop, that issue should be fixed now

@cseickel
Copy link
Contributor

Hi @andreadev-it, thanks for submitting this PR. I'm OK with it as is, but I'm going to wait and see what happens on Plenary before merging. If it is accepted there and turned into a utility function, it might be better to just reuse that final Plenary implementation here instead of duplicating it.

@cseickel
Copy link
Contributor

Hi @andreadev-it, it looks like the plenary one is stalled and I don't think it is relevant anymore because I have stopped using plenary for reading the filesystem.

Are you still interested in merging this?

I have to say, it seems like there should be more code changes/refactoring to make this work universally. I did a reference search for is_windows and came up with a lot of results with inconsistent methods of adjusting paths.

@andreadev-it
Copy link
Author

Hi @cseickel ,
Sorry for being out of touch these days...
From what I've experienced, enabling the shellslash option on windows causes way more harm than good, and I personally would not suggest it to anyone.
I agree that there might be the need to do more changes than these to allow the use of shellslash without generating issues. Actually, From what I've seen when I had shellslash enabled, even changing the code accordingly doesn't always work, since neovim also has a weird way of handling shellslash, IIRC.

Personally, I've stopped using shellslash, but if you think there might be the need to adjust it, I can look it up a little deeper and try to come up with a better fix

@cseickel
Copy link
Contributor

No problem @andreadev-it. No one else has asked for shellslash related improvements, so if you are no longer using it then I will close this PR.

@cseickel cseickel closed this Oct 31, 2022
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