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
utils/analytics: cleanup test-bot analytics. #17154
Conversation
Library/Homebrew/utils/analytics.rb
Outdated
step_command_short.split | ||
.map { |arg| arg.sub(/=.*/, "=") } | ||
.partition { |arg| !arg.start_with?("-") } | ||
command = (command_and_package + options.sort).join(" ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to do options.sort
then we may also want to deal with --option foo
flags (since we only currently handle --option=foo
flags by stripping the foo
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlocab If this is overkill: we could probably pull it all out. We don't use any long flags in brew test-bot
like this now, even moreso after the filtering in Homebrew/homebrew-test-bot#1043
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems overkill TBH. Better to pull it out, especially since I think the earlier call to partition
also means that the foo
in --flag foo
gets put in command_and_package
, so not sure it really does what's needed
Sort options to ensure consistent ordering and improve readability. Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
92ca38f
to
414e221
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok to merge after #17154 (comment)
@carlocab Whoops, auto-merge, follow-up incoming. |
#17154 (comment) was ignored by auto-merge, whoops.
Sort options to ensure consistent ordering and improve readability.