Skip to content

feat(__load_completion): search more paths based on the command location #696

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Oct 16, 2022
Merged

feat(__load_completion): search more paths based on the command location #696

merged 8 commits into from
Oct 16, 2022

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented Feb 12, 2022 •
edited
Loading

No description provided.

@akinomyoga akinomyoga closed this Feb 12, 2022
@akinomyoga akinomyoga deleted the extend-search-paths branch February 12, 2022 12:34
@akinomyoga
Copy link
Collaborator Author

I'm sorry. I just made a mistake. I intended to open a PR in my local repository for testing purpose.

@akinomyoga akinomyoga changed the title Extend search paths (CI test) [mistake] Extend search paths Feb 12, 2022
@akinomyoga akinomyoga changed the title [mistake] Extend search paths [mistake] Extend search paths of __load_completion Feb 12, 2022
@akinomyoga akinomyoga restored the extend-search-paths branch February 24, 2022 02:27
@akinomyoga akinomyoga changed the title [mistake] Extend search paths of __load_completion feat(__load_completions): search more paths based on the command location Feb 27, 2022
@akinomyoga akinomyoga changed the title feat(__load_completions): search more paths based on the command location feat(__load_completion): search more paths based on the command location Feb 27, 2022
@akinomyoga akinomyoga reopened this Jun 20, 2022
@scop
Copy link
Owner

scop commented Jun 28, 2022

Just wondering if this was reopened on purpose, at least there are some conflicts to be resolved...

@akinomyoga akinomyoga force-pushed the extend-search-paths branch from d0ae589 to 41114d1 Compare June 28, 2022 14:43
@akinomyoga
Copy link
Collaborator Author

Ah, sorry for confusing you. I reopened this on purpose, rebased it, and planned to add a description, but I became busy and left it in the current status [Note: GitHub seems to disallow/unrecognize the rebasing of the closed PRs, so I needed to reopen the issue before I push the rebased commits]. I had actually already completed the rebasing in my local repository but intended to push it after I add descriptions of the PR. However, I forgot the details of the PRs so need to take the time to look at it again.

Now I have pushed the rebased commits, but it's not ready for review because I haven't added descriptions of the PR.

@akinomyoga akinomyoga marked this pull request as draft June 28, 2022 14:47
@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Jun 28, 2022 •
edited
Loading

I have converted this to Draft PR. I'll later add the description and make it again a (normal) PR.

The original reason that I decided to reopen the PR at this timing is that I wanted to use _comp_split contained in this branch for another failglob fix 76402f7.

@akinomyoga akinomyoga force-pushed the extend-search-paths branch from 41114d1 to f72afca Compare August 28, 2022 02:19
@akinomyoga akinomyoga mentioned this pull request Sep 4, 2022
Merged
@akinomyoga akinomyoga mentioned this pull request Sep 18, 2022
Closed
@akinomyoga akinomyoga marked this pull request as ready for review September 18, 2022 23:29
@akinomyoga
Copy link
Collaborator Author

I think this is the time to revisit this PR. I actually forgot about the details of the backgrounds, but now I think the code/doc change tells everything essential about the change.

@akinomyoga
Copy link
Collaborator Author

I picked the changes of #823 (in commit ae26eaf).

scop
scop requested changes Sep 20, 2022
View reviewed changes
Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support the general idea here, but I believe we should adjust the search order a bit, and I'm also worried about how complex the docs are becoming (most of the comments arise from that, I hope you don't take them too negative).

@akinomyoga akinomyoga force-pushed the extend-search-paths branch from ae26eaf to 849bf32 Compare October 2, 2022 22:41
nurupo and others added 5 commits October 4, 2022 08:27
@nurupo @akinomyoga
docs(README): describe the local installation of external completions
530017d 
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
@akinomyoga @scop
fix(README): elaborate description of search order
be0085a 
* doc(README): rephrase description of the search-order change
* fix(README): mention `$PREFIX/sbin` in the search path
* fix(README): fix a missing word "visible"
* doc(README): describe the search order

Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
@akinomyoga
feat(__load_completion): look more places for completion files
12cff41 
* now multiple paths can be specified to BASH_COMPLETION_USER_DIR
  separated by colons.
* search the completion file with the installation prefix extracted by
  $(_realcommand CMD).
* search completion files with the installation prefixes extracted
  from $PATH.
@scop @akinomyoga
feat(__load_completion): load in-tree before system data dirs
c7d526d 
This swaps the lookup order of in-tree (relative to the main script) and
system data dirs. By checking in-tree completions first, before system
data dirs, we support setups running directly from a git clone better,
and have a more robust setup in any case.

The main script and in-tree completions should stay in sync. There might
for example be internal API changes between releases which wouldn't work
if the main `bash_completion` and the set of completions we ship were
from different versions, e.g. from git and a release.

Refs scop#537 (reply in thread)

* fix(__load_completion): swap order of in-tree and bundled completions
@akinomyoga
fix(__load_completion): lower the precedence of the completion "_cmd"
1984040
@akinomyoga akinomyoga force-pushed the extend-search-paths branch from 02926c0 to 1984040 Compare October 3, 2022 23:27
@akinomyoga
Copy link
Collaborator Author

rebased & squashed

@scop
Copy link
Owner

scop commented Oct 16, 2022

I took the liberty to add some more test coverage in 388a8ce and b22af05, I think we're good to go with this now; I hope I didn't break anything.

Thanks!

@scop scop merged commit 199851f into scop:master Oct 16, 2022
@nurupo nurupo mentioned this pull request Oct 17, 2022
Closed
@akinomyoga akinomyoga deleted the extend-search-paths branch October 19, 2022 08:12
@akinomyoga
Copy link
Collaborator Author

Thank you for reviewing and merging the PR!

@scop
Copy link
Owner

scop commented Oct 23, 2022

I believe (but haven't checked thoroughly) that 1984040 here may have caused #836

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scop scop scop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akinomyoga @scop @nurupo