[HooksManager] Show the name of the source for each hook that is run in verbose mode #2830

segiddins · 2014-11-14T00:31:57Z

Closes #2639.
Closes #2640.
Closes #2819.

Huge thanks to @alloy for the source_location hint! (feel free to say this all is too gross to include)

\c @kylef would appreciate some help making sure this is a robust solution

alloy · 2014-11-14T09:15:14Z

lib/cocoapods/hooks_manager.rb

+                  gem_name = (gem_spec && gem_spec.name) || File.basename(file, '.rb')
+                end
+
+                UI.message "- #{gem_name_lambda.call}" do


The string is always interpolated, regardless of whether or not the string will be used, so this always runs.

alloy · 2014-11-14T09:20:14Z

While I can appreciate the beauty of just a gem’s name, this will only be printed in verbose mode and verbose mode is meant to be able to easily find issues. So while the gem’s name helps a bit, it doesn’t help people that have a hard time figuring out where (on disk) that means an issue might lie.

So how about just printing the full info from source_location instead? This means there is no time spent on finding gem names and should make things easier to debug, also when users paste their output for us to read. E.g.

- Running Slather hooks
  - Defined at `/Library/Ruby/Gems/2.0.0/gems/Slather-1.2.3/lib/cocoapods_plugin.rb:42`

segiddins · 2014-11-14T09:39:16Z

Sure, that just wasn't in the initial issue :)

-Samuel E. Giddins

On Nov 14, 2014, at 1:20 AM, Eloy Durán notifications@github.com wrote:

While I can appreciate the beauty of just a gem’s name, this will only be printed in verbose mode and verbose mode is meant to be able to easily find issues. So while the gem’s name helps a bit, it doesn’t help people that have a hard time figuring out where (on disk) that means an issue might lie.

So how about just printing the full info from source_location instead? This means there is no time spent on finding gem names and should make things easier to debug, also when user's paste their output for us to read. E.g.

Running Slather hooks
- Defined at /Library/Ruby/Gems/2.0.0/gems/Slather-1.2.3/lib/cocoapods_plugin.rb:42
  —
  Reply to this email directly or view it on GitHub.

alloy · 2014-11-14T09:43:53Z

Hmm, so what was that initial issue then? :)

@kylef Didn’t leave a motivation in his ticket #2639, but as it mentions --verbose I assumed it was for debugging purposes.

kylef · 2014-11-14T11:10:41Z

My motivation was purely debugging purposes. Having a strange issue where a CocoaPods plugin wasn't being fired properly on Travis and was a little difficult to figure out why. This would make it clear if the hook was registered and executed with --verbose output.

kylef · 2014-11-14T11:12:44Z

I think I agree with @alloy, this can be simplified quite a bit by printing the path instead of the gem name. I initially used gem name in the existing issue because I though it would be easy to get until I looked into this. The path might even be preferred because it shows you the version and the exact location of the plugin if multiple versions are installed.

segiddins · 2014-11-15T00:21:13Z

While I'm at it, I'm just going to implement #2640.

segiddins · 2014-11-15T01:57:32Z

Instead of simplifying things, I just decided to add a whole new feature 😜

segiddins · 2014-11-15T02:11:46Z

I have no clue how my changes couldve broken the integration specs. @AliSoftware any ideas?

AliSoftware · 2014-11-15T09:35:02Z

Please not again! 😭 😭 I just passed hours fixing them last night… Your turn now! 😉

kylef · 2014-11-15T13:03:37Z

CHANGELOG.md

@@ -6,6 +6,12 @@ To install release candidates run `[sudo] gem install cocoapods --pre`

 ## Master

+##### Enhancements
+
+* Show the name of the source for each hook that is run in verbose mode.  


This should include the fact we only run the hooks for plugins defined in Podfile. I think we should show an example of how you would do this.

Perhaps this is now a breaking change as it breaks previous behaviour (like sources?

agree on breaking, considering how few plugins use the hooks right now we could list all the known ones in there.

Yeah I just never added a changelog entry.

segiddins · 2014-11-19T02:04:55Z

@kylef can we scope this for 0.36?

kylef · 2014-11-19T09:00:56Z

@segiddins Yes, I was already thinking this would go into 0.36.

kylef · 2014-11-21T00:01:09Z

Depends on #2819

@alloy

…in verbose mode Huge thanks to @alloy for the `source_location` hint! The name calculation is wrapped in a lambda so it's not called when it won't be printed out

We're incorrectly comparing the hook name instead of the plugin name in the whitelist

“Register slather for post install” vs “Register for post install slather”

[HooksManager] Show the name of the source for each hook that is run in verbose mode

alloy · 2014-11-24T12:40:59Z

@segiddins @kylef Just to be sure, the extra output is only shown in --verbose mode, right?

kylef · 2014-11-24T12:42:50Z

@alloy Correct, it uses UI.message.

alloy reviewed Nov 14, 2014
View reviewed changes

segiddins force-pushed the seg-hooks-plugin-name branch from 62dc3ea to 626dc3c Compare November 15, 2014 01:56

segiddins mentioned this pull request Nov 15, 2014

[Podfile] Allow the specification of plugins CocoaPods/Core#196

Merged

AliSoftware mentioned this pull request Nov 15, 2014

Broken integration specs #2832

Closed

kylef reviewed Nov 15, 2014
View reviewed changes

segiddins force-pushed the seg-hooks-plugin-name branch from 626dc3c to cf9d6f6 Compare November 15, 2014 19:32

kylef added this to the 0.36.0 milestone Nov 19, 2014

kylef self-assigned this Nov 21, 2014

segiddins added 3 commits November 20, 2014 16:24

[HooksManager] Show the name of the source for each hook that is run …

f637756

…in verbose mode Huge thanks to @alloy for the `source_location` hint! The name calculation is wrapped in a lambda so it's not called when it won't be printed out

Allow the specification of plugins in the podfile

cd9a2b8

[CHANGELOG] Add entry for podfile specification of plugins

24cc2d8

segiddins force-pushed the seg-hooks-plugin-name branch from c10976c to 24cc2d8 Compare November 21, 2014 00:30

segiddins mentioned this pull request Nov 21, 2014

Update to latest CLAide #2819

Closed

kylef added 2 commits November 22, 2014 00:21

[HooksManager] Fix whitelisted plugins hooks not fired

b9ca765

We're incorrectly comparing the hook name instead of the plugin name in the whitelist

[HooksManager] Change order of arguments for register

099e338

“Register slather for post install” vs “Register for post install slather”

kylef mentioned this pull request Nov 22, 2014

[HooksManager] Add plugin whitelisting #2865

Closed

kylef force-pushed the seg-hooks-plugin-name branch from 064cf20 to 099e338 Compare November 22, 2014 00:55

Update integration fixtures

928da4f

kylef added a commit that referenced this pull request Nov 22, 2014

Merge pull request #2830 from CocoaPods/seg-hooks-plugin-name

de33fb2

[HooksManager] Show the name of the source for each hook that is run in verbose mode

kylef merged commit de33fb2 into master Nov 22, 2014

kylef deleted the seg-hooks-plugin-name branch November 22, 2014 12:17

[HooksManager] Show the name of the source for each hook that is run in verbose mode #2830

[HooksManager] Show the name of the source for each hook that is run in verbose mode #2830

Uh oh!

Conversation

segiddins commented Nov 14, 2014

Uh oh!

alloy Nov 14, 2014

Choose a reason for hiding this comment

Uh oh!

alloy commented Nov 14, 2014

Uh oh!

segiddins commented Nov 14, 2014

Uh oh!

alloy commented Nov 14, 2014

Uh oh!

kylef commented Nov 14, 2014

Uh oh!

kylef commented Nov 14, 2014

Uh oh!

segiddins commented Nov 15, 2014

Uh oh!

segiddins commented Nov 15, 2014

Uh oh!

segiddins commented Nov 15, 2014

Uh oh!

AliSoftware commented Nov 15, 2014

Uh oh!

kylef Nov 15, 2014

Choose a reason for hiding this comment

Uh oh!

orta Nov 15, 2014

Choose a reason for hiding this comment

Uh oh!

segiddins Nov 15, 2014

Choose a reason for hiding this comment

Uh oh!

segiddins Nov 15, 2014

Choose a reason for hiding this comment

Uh oh!

segiddins commented Nov 19, 2014

Uh oh!

kylef commented Nov 19, 2014

Uh oh!

kylef commented Nov 21, 2014

Uh oh!

alloy commented Nov 24, 2014

Uh oh!

kylef commented Nov 24, 2014

Uh oh!

Uh oh!