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

Fix: Completions should use custom install location #48

Conversation

Bryan2333
Copy link
Contributor

No description provided.

@Bryan2333 Bryan2333 changed the title Make completions work with custom install location Some improvements for plugin Mar 29, 2024
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
@Bryan2333
Copy link
Contributor Author

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 reitzig self-assigned this Apr 1, 2024
@reitzig
Copy link
Owner

reitzig commented Apr 1, 2024

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 4c15f15, and subsequently the script in 66e2f1e
  2. Does that solve a specific issue or is it just a boyscout action? Why -r?

@reitzig reitzig changed the title Some improvements for plugin Fix: Completions should use custom install location Apr 1, 2024
@Bryan2333
Copy link
Contributor Author

  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
Copy link
Owner

reitzig commented Apr 3, 2024

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 c68660a.

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 reitzig changed the base branch from main to fix/completion-for-custom-install-path April 3, 2024 19:53
@reitzig
Copy link
Owner

reitzig commented Apr 3, 2024

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 reitzig closed this Apr 3, 2024
@reitzig
Copy link
Owner

reitzig commented Apr 3, 2024

Released with v2.1.0, thanks!

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.

None yet

2 participants