Rename apiHost → site and DataDogReporter → DatadogReporter#100
Rename apiHost → site and DataDogReporter → DatadogReporter#100
apiHost → site and DataDogReporter → DatadogReporter#100Conversation
Mr0grog
force-pushed
the
realign-some-names
branch
from
4c29e26 to
657f3da
Compare
I’m going to see about doing #85 tomorrow, but will probably make that the last change for now until I hear more feedback on this question of breaking changes and release plans. |
This renames the `apiHost` option for `init()` and `BufferedMetricsLogger` to `site` in order to better align with Datadog docs and official packages. The old name is kept for now, but logs a deprecation warning to the console (regardless of the user's DEBUG setting) the first time it is used. It also adds tooling for logging similar deprecation messages and centralizes debug logging so we don't have to worry about typos when people define the debug logger in a module.
As a followup to #98, rename `DataDogReporter` to `DatadogReporter` (no capital "D" in the middle). I didn't add a deprecation warning here because we haven't yet had a release that exposes this class publicly, so nobody could have been using it before.
Mr0grog
force-pushed
the
realign-some-names
branch
from
657f3da to
cad565d
Compare
|
Rebased this to resolve merge conflicts. We wound up shipping a release that publicly exposes I think the straightforward way to deprecate is with a subclass: class DataDogReporter extends DatadogReporter {
constructor(apiKey, appKey, site) {
logDeprecation(
'DataDogReporter has been renamed to DatadogReporter ' +
'(lower-case D in "dog"); the old name will be removed in a ' +
'future release.'
);
super(apiKey, appKey, site);
}
}On the other hand, I’m not sure this really minor change is worthwhile in the way that |
|
@ErikBoesen did you have any thoughts on this? In particular, whether to rename If not, I’m going to figure out how much I want to merge this and see about pushing out a v0.11.0 release later in the week (so the built-in type annotations are available, hopefully solving things like #109). |
|
Frankly I trust your judgment on this, considering I don't actually use this package myself anymore. I would lean towards the renaming to adhere to Datadog's branding standards. |
|
👍 That's helpful feedback! |
Back in #100, I renamed the `apiHost` option to `site` (in order to match up with Datadog's own docs and terminology), but forgot to rename the corresponding environment variable. This deprecates `DATADOG_API_HOST` in favor of `DATADOG_SITE` (as should have been done before). We'll remove support for the old env var when we remove `apiHost`.
Back in #100, I renamed the `apiHost` option to `site` (in order to match up with Datadog's own docs and terminology), but forgot to rename the corresponding environment variable. This deprecates `DATADOG_API_HOST` in favor of `DATADOG_SITE` (as should have been done before). We'll remove support for the old env var when we remove `apiHost`.
2 participants
In some other spots, I’ve noted that it might be good to rename
apiHosttositeso that it matches the Datadog documentation and official libraries, which all call this concept thesite. This does so by adding a newsiteoption and deprecatingapiHost(so it still works, but prints a warning message) and having it setsiteunder the hood.While I was at it, I followed the lead of #59 and #98 and renamed
DataDogReportertoDatadogReporter(no capital "D" in the middle). I didn’t do a deprecation here since there hasn’t yet been a release where it’s publicly visible. But we could do a deprecation instead if we want.This also builds on some of the tooling in #99, so if we want to land this, it needs to be done a little more carefully so everything merges correctly.(Edit: updated to merge cleanly.)@ErikBoesen what do you think of these changes? I think the first one is a good change that makes things easier to understand for new users, although neither one is really necessary.
More broadly, I’ve been holding off on implementing the breaking changes (#83 and part of #84 both involve subtle breakage) in case you want to cut a non-breaking 0.10.2 release first. The deprecation stuff here probably isn’t useful if you aren’t interested in that.