Open last modified file with spaces in path #388
Conversation
|
Hey @kriber82 thank you for your contribution and also the well described issue :) I will have a look at it later, when i have some time. But my first impression is, that it looks good :) |
|
closes #387 |
| @@ -40,7 +40,11 @@ func openCommandFor(c config.Configuration, filepath string) (string, []string) | |||
| if !c.IsOpenCommandGiven() { | |||
| return "", []string{} | |||
| } | |||
| split := strings.Split(injectCommandWithMessage(c.OpenCommand, filepath), " ") | |||
There was a problem hiding this comment.
I think we should be able to come up with a solution that does not need replacement with an arbitrary placeholder.
E.g. If we split the OpenCommand by space first (even before injecting the filepath), and only then inject the filepath into each of the pieces.
There was a problem hiding this comment.
As I see it, splitting OpenCommand before injecting the filepath would require a special version of injectCommandWithMessage that works on the list of pre-split arguments. Otherwise, the "single or no placeholder"-semantics of injectCommandWithMessage could not be enforced here.
If we drop this constraint here and assume that there is exactly one %s in OpenCommand, a simple strings.ReplaceAll on each split part (command argument) would do the trick, but that would modify existing behavior and duplicate parts of it.
Both of those solutions don't seem better to me than my proposed solution. Do you prefer one of those, or maybe have a better idea how to solve this?
As I contemplated a better solution, I came to the following insights and ideas:
- The problem here seems to be, that
injectCommandWithMessageis not aware of the fact, that commands may have arguments that contain spaces - Technically, this could also affect the other calls of
injectCommandWithMessage - A cleaner solution would probably have to model the difference between arguments and "words separated by spaces" explicitly.
- This might need to happen as early as parsing the config, as users might want to use double quotes in the config to mark arguments that contain spaces
- I would not be comfortable doing this kind of remodeling in an unfamiliar code-base (mob.sh) and foreign language (go)
- I would, however, be willing to pair- or ensemble-program this with someone more familiar whit both the codebase and go
Which path forward would you prefer @gregorriegler ?
| assertGitStatus(t, GitStatus{ | ||
| "file with spaces.txt-1": "??", | ||
| }) | ||
| } |
There was a problem hiding this comment.
Thank you @kriber82 for helping improve this tool ❤️. Looking forward to merge your PR. Please see my review comments where I propose a minor improvement.
|
I will incorporate your feedback, when I find time, which will probably be sometime next week. |
|
Hi @kriber82 do you think you will find the time for making the improvements? |
|
Sorry for the huge delay. Will try to do it within the next days. |
kriber82
force-pushed
the
open-last-modified-file-with-spaces-in-path
branch
from
4b40bc0
to
dd6cb23
Compare
This is my attempt to allow opening last modified files when the project path on disk contains spaces. I also opened issue #387 with further details.
The test I added works with a last modified file containing spaces instead of a project path containing spaces, as this was way easier to set up for me and seems to exhibit the same behavior.
As I have no prior experience with Go, I assume my code is not idiomatic. Also, I have not yet extracted functions yet. Therefore, feedback about how my code better fits into the codebase is greatly appreciated.