Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@daviwil
Copy link
Contributor

@daviwil daviwil commented Jun 25, 2018

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 nightly in the Atom version just as we do for beta, 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

expect(url).toContain('aiid=sushi')
})

it('reports an unknown release channel', async () => {
Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "nonstandard" or "unrecognized"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 'unrecognized'

expect(url).toContain('aiid=sushi')
})

it('reports an unknown release channel', async () => {
Copy link
Contributor

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')
})
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing!

Copy link
Contributor Author

@daviwil daviwil left a 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

expect(url).toContain('aiid=sushi')
})

it('reports an unknown release channel', async () => {
Copy link
Contributor Author

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')
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing!

Copy link

@annthurium annthurium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@jasonrudolph
Copy link
Contributor

🚀

@daviwil
Copy link
Contributor Author

daviwil commented Jun 25, 2018

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?

@annthurium
Copy link

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.

@daviwil
Copy link
Contributor Author

daviwil commented Jun 25, 2018

Cool! I'll go ahead and do a patch bump in the meantime. Thanks a bunch!

@daviwil daviwil merged commit 18d726d into master Jun 25, 2018
@daviwil daviwil deleted the dw-release-channels branch June 25, 2018 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants