reitzig / sdkman-for-fish

Adds support for SDKMAN! to fish
MIT License
280 stars 13 forks source link

Fix: Completions should use custom install location #48

Closed Bryan2333 closed 2 months ago

Bryan2333 commented 3 months ago

I improved the plugin, here are some points:

  1. Make the completions work with the custom install location. The sdkman install location is hardcoded in the original completion file.
  2. Replace the tr command with fish shell builtin command
reitzig commented 3 months ago

Ahoi, thanks for your contribution!

  1. Seems reasonable to me; good catch! In order to protect against regressions, can you add a test that fails without your change, please?
    • Note to self: Missed completion tests in 4c15f15ced5a67e00b161b2a5e83df7d660405d3, and subsequently the script in 66e2f1e6238295cc985f86ee00382fc4daec10cd
  2. Does that solve a specific issue or is it just a boyscout action? Why -r?
Bryan2333 commented 3 months ago
  1. I'm afraid not, because I'm not familiar with the test framework.
  2. No, I just want to replace the tr command. And yes, it is fine to use the original one. Sorry for bringing confusion.
reitzig commented 2 months ago

I'm afraid not, because I'm not familiar with the test framework.

Too bad. I had hoped you might be willing to try and copy-paste something together, if my test setup was simple enough ... oh well. It was just copy past, see c68660ab8051b041309c9ba97923475218347587.

No, I just want to replace the tr command. And yes, it is fine to use the original one. Sorry for bringing confusion.

Gotcha, no worries. I'm fine with avoiding tr, but I think we can drop -r?

reitzig commented 2 months ago

Couldn't figure out how to push to your PR (did you check the box to allow maintainer edits? 🤔) so I'm closing this in favor of #51.

Thanks again!

FWIW:

I think we can drop -r

I checked; no, we can't. Apparently, string replace only handles escapes such as \n properly in regex-mode. 🙄 The more you know!

reitzig commented 2 months ago

Released with v2.1.0, thanks!