add `brew analytics` command #173

Merged
merged 1 commit into from May 1, 2016

Projects

None yet

4 participants

@xu-cheng
Member
xu-cheng commented May 1, 2016

No description provided.

@xu-cheng xu-cheng added the discussion label May 1, 2016
@MikeMcQuaid
Member

Looks great to me. Mind updating https://github.com/Homebrew/brew/blob/master/share/doc/homebrew/Analytics.md to note to use it instead of the commands?

@MikeMcQuaid MikeMcQuaid commented on the diff May 1, 2016
Library/Homebrew/cmd/analytics.rb
@@ -0,0 +1,44 @@
+#: * `analytics` [`state`]:
@MikeMcQuaid
MikeMcQuaid May 1, 2016 Member

Would be good to point to the analytics documentation in here and use the language from there (anonymous user behaviour analytics)

@DomT4
Contributor
DomT4 commented May 1, 2016

Heh, was talking about this with Mike the other day. You beat me to actually getting the work done ๐Ÿ˜ƒ.

@UniqMartin UniqMartin and 1 other commented on an outdated diff May 1, 2016
Library/Homebrew/cmd/analytics.rb
@@ -0,0 +1,44 @@
+#: * `analytics` [`state`]:
+#: Display analytics state.
+#:
+#: * `analytics` `on`|`off`:
@UniqMartin
UniqMartin May 1, 2016 Contributor

This should be

#:  * `analytics` (`on`|`off`):

to be consistent with other documentation we have where there's a list of mandatory alternatives.

@UniqMartin UniqMartin and 1 other commented on an outdated diff May 1, 2016
Library/Homebrew/cmd/analytics.rb
@@ -0,0 +1,44 @@
+#: * `analytics` [`state`]:
+#: Display analytics state.
+#:
+#: * `analytics` `on`|`off`:
+#: Turn on/off Homebrew's analytics.
+#:
+#: * `analytics` `regenerate-uuid`:
+#: Regenerate UUID used in Homebrew's analytics.
+
+module Homebrew
+ def analytics
+ config_file = HOMEBREW_REPOSITORY/".git/config"
+
+ case ARGV.named.first
+ when nil, "state"
@UniqMartin
UniqMartin May 1, 2016 Contributor

I'd like to see every when clause of this case to be refactorded into a separate method to keep Homebrew.analytics short and readable. And I wonder if we should also complain when passing more than one named argument. (Let's try to be more strict in our argument handling for new brew commands we introduce.)

@xu-cheng
xu-cheng May 1, 2016 Member

I keep it is for now. Although I think it's better to put them into separate methods, I think we should wait when we implement command in class. Otherwise, it's too easy to pollute global namespace.

@DomT4
Contributor
DomT4 commented May 1, 2016

Part of me wonders if HOMEBREW_NO_ANALYTICS_THIS_RUN shouldn't be set permanently for this command. It seems a little counterproductive to track anonymous usage at the same time someone is considering whether to stay opted in/out.

@UniqMartin
Contributor

Good work! It would be great to also update the shell completion of your shell to work with this new command.

Part of me wonders if HOMEBREW_NO_ANALYTICS_THIS_RUN shouldn't be set permanently for this command. It seems a little counterproductive to track anonymous usage at the same time someone is considering whether to stay opted in/out.

We could consider adding it to the list of commands exempt from tracking (we're already excluding brew commands).

@DomT4
Contributor
DomT4 commented May 1, 2016 edited

Your suggestion sounds good to me, FWIW.

Edit - Reworded for clarity.

@xu-cheng
Member
xu-cheng commented May 1, 2016 edited

Part of me wonders if HOMEBREW_NO_ANALYTICS_THIS_RUN shouldn't be set permanently for this command. It seems a little counterproductive to track anonymous usage at the same time someone is considering whether to stay opted in/out.

We could consider adding it to the list of commands exempt from tracking (we're already excluding brew commands).

I think we should do this in another PR. Because there are many other commands (e.g, --prefix) belong to this category. At the same time, I'm wondering should we only send screen view for only selective commands as personally I don't feel that such statistics can help us much.

Addressed other comments.

@MikeMcQuaid
Member

Currently brew analytics off will be tracked and create the UUID file, no? Also missing the change to the analytics documentation.

@xu-cheng
Member
xu-cheng commented May 1, 2016

Currently brew analytics off will be tracked and create the UUID file, no?

Good point, fixed this.

Also missing the change to the analytics documentation.

What do you mean? Isn't it already in there? https://github.com/Homebrew/brew/pull/173/files#diff-de62681d221dc414a999cf7eec2f0f36

@MikeMcQuaid
Member

@xu-cheng Sorry, I missed that before, my mistake.

@xu-cheng
Member
xu-cheng commented May 1, 2016

NP, just want to check if I missing anything.

Ready to ship?

@UniqMartin UniqMartin commented on an outdated diff May 1, 2016
Library/Homebrew/utils/analytics.sh
@@ -72,10 +70,10 @@ report-analytics-screenview-command() {
# Don't report commands used mostly by our scripts and not users.
# TODO: list more e.g. shell completion things here perhaps using a single
# script as a shell-completion entry point.
- if [[ "$HOMEBREW_COMMAND" = "commands" ]]
- then
- return
- fi
+ case "$HOMEBREW_COMMAND" in
+ --prefix|analytics|command|commands)
+ return ;;
+ esac
@UniqMartin
UniqMartin May 1, 2016 Contributor

Minor nit, but so far we seem to prefer either

case "$HOMEBREW_COMMAND" in
  --prefix|analytics|command|commands)
    return
    ;;
esac

or

case "$HOMEBREW_COMMAND" in
  --prefix|analytics|command|commands) return;;
esac
@UniqMartin
Contributor

Ready to ship?

One final (and very minor) remark, but otherwise ๐Ÿ‘.

@xu-cheng xu-cheng merged commit 98aff27 into Homebrew:master May 1, 2016

1 check passed

default Build finished.
Details
@xu-cheng xu-cheng deleted the xu-cheng:analytics branch May 1, 2016
@UniqMartin UniqMartin referenced this pull request May 1, 2016
Closed

analytics: add bash completion #178

4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment