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

install tap cmd completions #1708

Merged
merged 1 commit into from Feb 27, 2017

Conversation

Projects
None yet
2 participants
@joshka
Copy link
Contributor

joshka commented Dec 20, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Taps can include completion scripts for external commands under
completions/bash, completions/fish, or completions/zsh. brew tap
will automatically install these into the correct directories during
install.

See Homebrew/homebrew-services#73 for a concrete need for completions in a repo.

This PR follows on from #1195 but github didn't pick up the extra commits automatically as part of the previous PR (possibly because of my local rebase to pickup changes made later).

@joshka joshka force-pushed the joshka:feature/install-tap-cmd-completions branch 2 times, most recently from 5a12a0b to 3b5e895 Dec 21, 2016

@@ -76,7 +76,8 @@ def list
lib/ruby/gems/[12].*
lib/ruby/site_ruby/[12].*
lib/ruby/vendor_ruby/[12].*
manpages/brew.1
manpages/man1/brew.1
manpages/man1/brew-cask.1

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Dec 23, 2016

Member

Are there taps using non-.1 manpages (or wanting to)?

This comment has been minimized.

@joshka

joshka Dec 24, 2016

Contributor

This allows us to handle the man pages just as a generic nested folder for install / uninstall rather than specifically handling the man case. I'd like to leave this like this for simplicity. It makes it obvious where a non.1 case would belong (arguablely brew should be .8)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Dec 24, 2016

Member

Do we have any non-.1 cases currently, though? If not, I'd rather leave as-is (or at least make that another PR).

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 15, 2017

Member

@joshka Ping on this, let's leave it as-is for now, please and save moving these around to another PR.

@@ -302,16 +302,11 @@ def migrate_legacy_repository_if_necessary
$stderr.puts e.backtrace
end

def link_completions_and_docs(repository = HOMEBREW_REPOSITORY)
def link_additional_files(repository = HOMEBREW_REPOSITORY)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Dec 23, 2016

Member

I think perhaps link_completions_manpages_and_docs would better signify the intent here.

This comment has been minimized.

@joshka

joshka Dec 24, 2016

Contributor

But what happens when we add a config file, some other related thing, ...

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Dec 24, 2016

Member
  1. YAGNI, 2) we can change the name then. link_additional_files doesn't really communicate anything.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 15, 2017

Member

@joshka Let's use link_completions_manpages_and_docs, thanks.

end
end

def install_manpages(path, command)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Dec 23, 2016

Member

Can we keep the naming of these to be (un)link_*? Also, probably worth splitting these into a new utils/link.rb or similar file.

This comment has been minimized.

@joshka

joshka Dec 24, 2016

Contributor

I like install because it tells the intent rather than the effect. How this occurs might change without too much hassle, but why it's happening remains the same

This comment has been minimized.

@joshka

joshka Dec 24, 2016

Contributor

Kinda agree on link.rb

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Dec 24, 2016

Member

Let's leave it as link but move these to link.rb 👍

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 15, 2017

Member

Again, let's leave these as link, thanks.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Dec 23, 2016

Nice work here so far. A few comments but looking good 👍

@@ -76,7 +76,8 @@ def list
lib/ruby/gems/[12].*
lib/ruby/site_ruby/[12].*
lib/ruby/vendor_ruby/[12].*
manpages/brew.1
manpages/man1/brew.1
manpages/man1/brew-cask.1

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 15, 2017

Member

@joshka Ping on this, let's leave it as-is for now, please and save moving these around to another PR.

@@ -302,16 +302,11 @@ def migrate_legacy_repository_if_necessary
$stderr.puts e.backtrace
end

def link_completions_and_docs(repository = HOMEBREW_REPOSITORY)
def link_additional_files(repository = HOMEBREW_REPOSITORY)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 15, 2017

Member

@joshka Let's use link_completions_manpages_and_docs, thanks.

end
end

def install_manpages(path, command)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 15, 2017

Member

Again, let's leave these as link, thanks.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 15, 2017

@joshka Ping on the changes/conflicts here, thanks.

@joshka

This comment has been minimized.

Copy link
Contributor

joshka commented Feb 15, 2017

@MikeMcQuaid I had a rather extended Christmas / new year vacation and then moved house and had to put this on the back burner. I'll take a look later today and get it back on track. :)

joshka added a commit to joshka/brew that referenced this pull request Feb 18, 2017

Address PR Homebrew#1708 review comments
Only handle man1 man pages for now
Rename link_additional_files to link_completions_manpages_and_docs
/ link_completions_and_manpages
Rename (un)install_manpages to (un)link_manpages
@joshka

This comment has been minimized.

Copy link
Contributor

joshka commented Feb 18, 2017

@MikeMcQuaid I've fixed up the review comments. I'm happy to rebase / squash / resolve conflicts once you've looked at it.

@joshka joshka force-pushed the joshka:feature/install-tap-cmd-completions branch from e308f29 to 1de4f84 Feb 18, 2017

joshka added a commit to joshka/brew that referenced this pull request Feb 18, 2017

Address PR Homebrew#1708 review comments
Only handle man1 man pages for now
Rename link_additional_files to link_completions_manpages_and_docs
/ link_completions_and_manpages
Rename (un)install_manpages to (un)link_manpages

@joshka joshka force-pushed the joshka:feature/install-tap-cmd-completions branch from 1de4f84 to 8ea6d37 Feb 18, 2017

@joshka

This comment has been minimized.

Copy link
Contributor

joshka commented Feb 18, 2017

I've rebased against master, squashed the WIP commits, and fixed the review item that I missed (the link functions are now in link.rb)

HOMEBREW_PREFIX/"share/man/man1", command)
link_completions(repository, command)
link_manpages(repository, command)
link_docs(repository, command)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 20, 2017

Member

Given this function is only called from here and only applies to Homebrew/brew I wonder if the indirection here is a little confusing (but have no strong views either way).

This comment has been minimized.

@joshka

joshka Feb 20, 2017

Contributor

I'm not sure that I understand your question. Are you talking about link_completions_manpages_and_docs() or link_docs() method here?
If it's the former, the method exists to DRY up the same code in the taps.
If it's the latter, the method exists for consistency with the other link_ methods.

Or perhaps you're suggesting to inline the calls to link_* at the call sites? I'm a fan of smaller methods rather than larger generally, and we need to call Tap.link_completions_and_manpages externally so I think for consistency this means this stays.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 20, 2017

Member

link_docs, yes. If you feel strongly: I don't so leave as-is.

@@ -0,0 +1,62 @@
def link_src_dst_dirs(src_dir, dst_dir, command, link_dir: false)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 20, 2017

Member

This should be in utils/link.rb and be inside a module Link

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 20, 2017

Member

Additionally, this method could be private.

This comment has been minimized.

@joshka

joshka Feb 23, 2017

Contributor

makes sense to me

end
end

def link_manpages(path, command)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 20, 2017

Member

I realise I'm changing my mind but could always be link_man1pages or something similar if you wanted to communicate this only refers to man1

This comment has been minimized.

@joshka

joshka Feb 23, 2017

Contributor

I think it makes enough sense as-is to leave it and only update the implementation if a situation presents that other man sections are needed. Adding manX would likely mean reorganizing a bunch of folders across various repos, so it would be a fairly heavy change that isn't just a matter of adding a new method

EOS
end

def unlink_src_dst_dirs(src_dir, dst_dir, unlink_dir: false)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 20, 2017

Member

This method could be private

@joshka joshka force-pushed the joshka:feature/install-tap-cmd-completions branch from 8ea6d37 to 46fd691 Feb 23, 2017

@joshka

This comment has been minimized.

Copy link
Contributor

joshka commented Feb 23, 2017

Addressed module change

@joshka joshka force-pushed the joshka:feature/install-tap-cmd-completions branch from 46fd691 to beedb2b Feb 24, 2017

@@ -0,0 +1,68 @@
module Link

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 26, 2017

Member
module Utils
  module Link

This comment has been minimized.

@joshka

joshka Feb 26, 2017

Contributor

Cool with me. I wasn't sure about the idiomatic ruby way on this was and the modules under /utils seemed to follow both approaches.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 26, 2017

Final bit of pedantry but otherwise looking good. Once that's addressed I'll give this some testing and get Homebrew/homebrew-services ready to go with it then 🚢.

Install tap command completions and manpages
Taps can include completion scripts for external commands under
`completions/bash`, `completions/fish`, or `completions/zsh`. `brew tap`
will automatically install these into the correct directories during
install.

@joshka joshka force-pushed the joshka:feature/install-tap-cmd-completions branch from beedb2b to 25396d9 Feb 26, 2017

@joshka

This comment has been minimized.

Copy link
Contributor

joshka commented Feb 26, 2017

Done. I note that while this PR adds more tests as an absolute number, the code coverage check on the diff reports that it's not quite enough. I think this is due to a large portion of this diff being a refactor of methods to a new module.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 27, 2017

Thanks for your work and patience here @joshka!

@MikeMcQuaid MikeMcQuaid merged commit 0a8c8f9 into Homebrew:master Feb 27, 2017

3 checks passed

codecov/patch 67.21% of diff hit (target 60.62%)
Details
codecov/project 60.71% (+0.09%) compared to e3f4701
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

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