Switch to reverse domain named keys for instrumentation #47
Conversation
|
@wagenet Thanks for your PR! This looks good to me, and I've tagged in @lawrencejones to take a look too. We should have a think about how we want the backwards compatibility to work here. I'm not sure if we should just cut the existing keys now rather than offer them in deprecated form. Obviously, in doing that we'd bump a major version and clearly document the change and how to up date. It is nice to offer a deprecation like this, but it introduces a bit of pain on this side and uncertainty about when we can remove it for what is a very small change. |
|
@timrogers my preference for deprecations is to include them in at least one non-breaking release before the breaking one. However, I don't actually use coach, so I don't have a strong opinion here. |
|
Hey @wagenet- thanks for this! One quick request- I've enabled builds for external forks so if you touch this commit and re-push, then it should trigger CircleCI. I like the idea of using the deprecation warnings to let people know in advance. Leaving a single major version to accommodate for others making the change seems sensible to me. With a green build, I'll be +1 on this. |
|
Apparently Rubocop doesn't like my PR. IMO, these are a bit aggressive, but given that it's not my project I can change things a bit. |
|
@wagenet I figure you're probably just bumped into some line length limits/done things differently with rspec than the rest of the project does. We have ~500k lines of Ruby at GC, and discovered early on that we preferred things consistent than variable! Hopefully it should be an easy fix. |
The conventional structure of ActiveSupport::Notification keys is "event.library". However, coach used "library.event". This commit adds support for the preferred style and deprecates the old one for removal in a future version.
|
@lawrencejones looks like we're green now. |
|
@wagenet Thanks! We’ll release this today. |
The conventional structure of ActiveSupport::Notification keys is
"event.library". However, coach used "library.event". This commit
adds support for the preferred style and deprecates the old one
for removal in a future version.