Skip to content

Bash Completion: fix bug in dash vs underscore (#780) #781

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

Conversation

jaredgrubb
Copy link

The generated script tries to call sub-functions that have hyphens in them instead of underscores.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

The generated script tries to call sub-functions that have hyphens in them
instead of underscores.
@jaredgrubb
Copy link
Author

@rgoldberg I see you've been adjusting completions lately. I want to make sure you see this PR.

@rgoldberg
Copy link
Contributor

rgoldberg commented May 17, 2025

@jaredgrubb FYI, my PR to redo completions using ToolIfnoV0 (#764) already fixes this issue.

It no longer replaces - with _.

I think that's a better solution, because, otherwise, a name conflict can exist for bash functions for subcommand a-b & a_b. It's unlikely, but possible that someone will have both, but functions for both would be named …a_b….

If you agree that my solution works better, and that my PR implements it, this PR should be closed.

@jaredgrubb
Copy link
Author

I think that sounds ok -- as long as that PR is likely to merge before the next release?
Otherwise, this PR is a very nice quick-fix for something that's broken today and your PR could rebase on top of it until it's ready.

@rgoldberg
Copy link
Contributor

@jaredgrubb I hope my PR will be merged before the next release. It & my other PRs seem ready to go. If there are issues wit migrating to ToolInfoV0, I can always extract my solution for this from that PR and submit it by itself.

Hopefully I'll hear back from the project members soon.

DO you think it makes sense to mark this PR as a draft so they don't merge it then mine, causing unnecessary merge issues, etc.?

Or, could you mention in the initial comment for this PR that a more complete solution already exists in #764 & can be extracted?

It just doesn't make sense to me to go for a quicker solution that has naming clash issues when the full solution is already available.

@natecook1000
Copy link
Member

@jaredgrubb Thanks for looking at this! I'm working on reviewing #764, and will make sure it lands before the next release, so that change will resolve the underlying issue.

@jaredgrubb
Copy link
Author

Sounds good! I look forward to the fix!

@rgoldberg
Copy link
Contributor

rgoldberg commented May 18, 2025

@jaredgrubb Thanks for noticing this, and for giving me a heads up. Sorry for having preempted your fix.

If you notice any other completion problems, please open more issues.

Many completion bugs are still extant, with many due to string quoting, escaping, and/or delimiting issues. I just haven't had the chance to find & fix them all yet.

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.

3 participants