-
Notifications
You must be signed in to change notification settings - Fork 32
Report arbitrary release channel names #92
Conversation
spec/metrics-spec.js
Outdated
| expect(url).toContain('aiid=sushi') | ||
| }) | ||
|
|
||
| it('reports an unknown release channel', async () => { |
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.
'unknown' may not be the best name for this because it should indicate an Atom version that doesn't conform to the expected pattern. Any thoughts?
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.
Maybe "nonstandard" or "unrecognized"?
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.
👍 'unrecognized'
spec/metrics-spec.js
Outdated
| expect(url).toContain('aiid=sushi') | ||
| }) | ||
|
|
||
| it('reports an unknown release channel', async () => { |
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.
Maybe "nonstandard" or "unrecognized"?
|
|
||
| let url = Reporter.request.mostRecentCall.args[0] | ||
| expect(url).toContain('aiid=sushi') | ||
| }) |
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.
@daviwil: Since we know for sure that we want to support a nightly release channel, how would feel about adding a test specifically for the nightly release channel?
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.
Sure thing!
daviwil
left a comment
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.
Thanks @jasonrudolph! Made those changes
spec/metrics-spec.js
Outdated
| expect(url).toContain('aiid=sushi') | ||
| }) | ||
|
|
||
| it('reports an unknown release channel', async () => { |
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.
👍 'unrecognized'
|
|
||
| let url = Reporter.request.mostRecentCall.args[0] | ||
| expect(url).toContain('aiid=sushi') | ||
| }) |
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.
Sure thing!
annthurium
left a comment
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.
looks good!
|
🚀 |
|
Thanks all! Think we'll have another release of metrics package within the next couple of weeks or should I go ahead and do a patch bump? |
|
I was planning on doing a release of the metrics package after I merge #91. That's a riskier change though, so it might make sense to do a patch bump with just your changes. Up to you, though. |
|
Cool! I'll go ahead and do a patch bump in the meantime. Thanks a bunch! |
Description of the Change
This change updates the release channel reporting logic to support arbitrary channel names by comparing the current Atom version against a regular expression. This will enable the new Atom Nightly release channel to be reported.
Alternate Designs
A simpler solution would just be to look for
nightlyin the Atom version just as we do forbeta, but this solution gives us the flexibility to have other release channels in the future with less effort.Benefits
We'll have usage metrics for other release channels!
Possible Drawbacks
None that I can think of.
Applicable Issues
atom/atom#17538