-
Notifications
You must be signed in to change notification settings - Fork 795
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
feat: validate metric names #468
feat: validate metric names #468
Conversation
Codecov Report
@@ Coverage Diff @@
## master #468 +/- ##
=========================================
+ Coverage 94.83% 95.2% +0.36%
=========================================
Files 130 125 -5
Lines 6524 6295 -229
Branches 556 519 -37
=========================================
- Hits 6187 5993 -194
+ Misses 337 302 -35
|
@@ -48,7 +48,7 @@ describe('Meter', () => { | |||
|
|||
describe('.getHandle()', () => { | |||
it('should create a counter handle', () => { | |||
const counter = meter.createCounter('name'); | |||
const counter = meter.createCounter('name') as CounterMetric; |
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.
Just curious, why do you need to use as
- Type assertion here? Earlier it used to work, Is it because we are returning noop implementation?
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.
Yes, the function createCounter
now returns types.Metric<types.CounterHandle>
where it used to return just CounterMetric
. The return type needed to be changed in order to allow returning the noop implementation which satisfies the type types.Metric<types.CounterHandle>
but is not a CounterMetric
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.
Got you. As a user, I have to use/do the same assertion? This looks little inconvenient. WDYT?
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.
No you don't, because users will only use the public interface. This is only done because the tests access private fields
Co-authored-by: Valentin Marchaud <[email protected]>
Which problem is this PR solving?
Short description of the changes