Skip to content

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

Closed
jaredgrubb wants to merge 1 commit into
apple:mainfrom
jaredgrubb:jaredgrubb-fix-bash-completion-subcommand
Closed

Bash Completion: fix bug in dash vs underscore (#780)#781
jaredgrubb wants to merge 1 commit into
apple:mainfrom
jaredgrubb:jaredgrubb-fix-bash-completion-subcommand

Conversation

@jaredgrubb

Copy link
Copy Markdown

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
Copy Markdown
Author

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

@rgoldberg

rgoldberg commented May 17, 2025

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
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
Copy Markdown
Collaborator

@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
Copy Markdown
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
Copy Markdown
Author

Sounds good! I look forward to the fix!

@rgoldberg

rgoldberg commented May 18, 2025

Copy link
Copy Markdown
Collaborator

@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