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

Let bash add or suppress trailing space #159

Merged
merged 1 commit into from Dec 9, 2016

Conversation

evanunderscore
Copy link
Collaborator

Fixes #50 and #151

@evanunderscore
Copy link
Collaborator Author

I know this may be somewhat controversial since it's a fairly fundamental shift in the interaction between argcomplete and the shell. I'm also not aware of exactly how this will affect shells like zsh or tcsh.

@codecov-io
Copy link

codecov-io commented Oct 22, 2016

Current coverage is 81.01% (diff: 100%)

Merging #159 into master will increase coverage by 0.09%

@@             master       #159   diff @@
==========================================
  Files             5          5          
  Lines           802        806     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            649        653     +4   
  Misses          153        153          
  Partials          0          0          

Powered by Codecov. Last update d17dd98...66060fb

@kislyuk
Copy link
Owner

kislyuk commented Oct 29, 2016

Thanks for the PR. I think I'm on board with this, but I'm still trying to understand the possible side effects (or lack thereof) of this.

Could you please rebase this on master so it's good to merge?

@evanunderscore
Copy link
Collaborator Author

As far as I can tell, there should be no difference when this is used from bash. However this will definitely impact anyone using the completions directly since they'll be expecting exact matches to have a space appended, so in that sense this is a backwards compatibility break. If you'd prefer, we could make this opt-in for now.

@evanunderscore
Copy link
Collaborator Author

I've updated this to account for the testing changes. In my opinion, merging this would require a major version bump since some code may rely on the trailing space that is being removed (assuming you're doing semver). Alternatively I could make it optional and have the bash scripts activate this change through an environment variable or some other mechanism.

@evanunderscore
Copy link
Collaborator Author

On reflection I think it's a better idea to make this optional. I'll make the appropriate changes.

@evanunderscore evanunderscore force-pushed the bash-nospace branch 2 times, most recently from e374818 to ee7b547 Compare December 3, 2016 02:54
@evanunderscore
Copy link
Collaborator Author

I've updated this so that the default remains unchanged and environments that are prepared to handle this for themselves can opt out. This is ready for another review.

@kislyuk
Copy link
Owner

kislyuk commented Dec 9, 2016

Looks good. Merging.

I'll play around with this and once I understand the implications (or lack thereof), I'll put my opinion here on whether it should be on by default.

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

3 participants