Skip to content
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

Add typings for ext submodules #545

Merged
merged 3 commits into from
May 1, 2019
Merged

Add typings for ext submodules #545

merged 3 commits into from
May 1, 2019

Conversation

Oblosys
Copy link
Contributor

@Oblosys Oblosys commented Apr 19, 2019

I needed some of these typings, so I created the others as well. The Datadog constants are typed as string, to avoid duplicating the values in the type definitions, but the constants from opentracing are imported, so their types are narrowed to the actual values. In case you want this to be consistent, those constants could be typed as string as well (or the Datadog constants could be narrowed to their actual values).

@rochdev
Copy link
Member

rochdev commented Apr 24, 2019

I would say to narrow the values similar to OpenTracing. Then it will be possible as a future improvement to also add auto completion for any built-in tag by defining accepted values.

.npmignore Outdated Show resolved Hide resolved
ext/formats.d.ts Outdated Show resolved Hide resolved
ext/index.d.ts Outdated Show resolved Hide resolved
ext/index.d.ts Show resolved Hide resolved
ext/priority.d.ts Show resolved Hide resolved
ext/tags.d.ts Outdated Show resolved Hide resolved
@Oblosys
Copy link
Contributor Author

Oblosys commented Apr 25, 2019

Added the changes as a separate commit for clarity, you can squash it if you agree with the changes.

rochdev
rochdev previously approved these changes May 1, 2019
Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

LGTM

@Oblosys Do you think these should be tested in test.ts? Maybe at least one entry per file just to validate they are picked up by the transpiler? Is it guaranteed that TypeScript will always pick up every .d.ts for a corresponding .js?

@rochdev rochdev added this to the 0.11.1 milestone May 1, 2019
@Oblosys
Copy link
Contributor Author

Oblosys commented May 1, 2019

Ah, there are tests, but they are in the docs/ folder :-) I've added some import statements to test the existence of all typed constants. TypeScript will indeed pick up the .d.ts files.

@rochdev rochdev merged commit 4b435cf into DataDog:master May 1, 2019
@Oblosys Oblosys deleted the add-ext-typings branch May 1, 2019 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants