-
Notifications
You must be signed in to change notification settings - Fork 97
Conversation
// A map mapping TagKey to to its respective TagValue. | ||
private readonly registeredTags: Map<TagKey, TagValue> = new Map(); | ||
|
||
constructor() {} |
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.
Can this empty constructor be removed?
/** | ||
* Inserts a key and value in the map if the map does not already contain the | ||
* key. | ||
* @param tagKey The tag key. |
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.
These @param
comments seem to be fully redundant with the parameter names. Can they be removed?
This comment applies to insert
, delete
and update
below too.
* @param tagKey The tag key. | ||
*/ | ||
delete(tagKey: TagKey): void { | ||
if (this.registeredTags.has(tagKey)) { |
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.
Do you need call to has
? Map.delete
just returns false
if the key was not deleted but otherwise is a no-op.
if (!isValidTagValue(tagValue)) { | ||
throw Error(`Invalid TagValue: ${tagValue.value}`); | ||
} | ||
this.registeredTags.set(tagKey, tagValue); |
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.
Do you need to check for a valid tag value here?
* @param tagKey A tag key to be updated. | ||
* @param tagValue The value to update the key to in the map. | ||
*/ | ||
update(tagKey: TagKey, tagValue: TagValue): void { |
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.
Why have separate insert
and update
functions and not just a set
function that does insert and update?
That would enable combining both functions and you also would not need the if (this.registeredTags.has(tagKey)) {
check.
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.
+1 to this. In Java we use this approach in TagContextBuilder.put()
.
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.
Done, thanks for the reference.
const invalidString = /[^\u0020-\u007e]/; | ||
|
||
// Max Length of a TagKey | ||
const MAX_LENGTH = 255; |
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.
Nit: could this be TAG_KEY_MAX_LENGTH
and then remove the comment?
* @return whether the name is valid. | ||
*/ | ||
export function isValidTagKey(tagKey: TagKey): boolean { | ||
if (tagKey === null || typeof tagKey === 'undefined') { |
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.
Could this be compacted to if (!tagKey) return false;
? Given that tagKey
needs to be an object with a valid name
, all falsely values should be invalid.
* @param tagValue the name to be validated. | ||
* @return whether the name is valid. | ||
*/ | ||
|
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.
Nit: should this extra line be removed?
*/ | ||
|
||
export function isValidTagValue(tagValue: TagValue): boolean { | ||
if (tagValue === null || typeof tagValue === 'undefined') { |
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.
Similar question here on whether it makes sense to use falsiness as a check here.
assert.equal(tags.size, 1); | ||
}); | ||
}); | ||
describe('update()', () => { |
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.
Nit, should this be delete()
?
1. removed redundant param comments. 2. removed empty constructor. 3. removed unnecessary map.has check. 4. renamed invalidString -> nonPrintableCharsRegex, MAX_LENGTH -> TAG_KEY_MAX_LENGTH
* @param tagKey A tag key to be updated. | ||
* @param tagValue The value to update the key to in the map. | ||
*/ | ||
update(tagKey: TagKey, tagValue: TagValue): void { |
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.
+1 to this. In Java we use this approach in TagContextBuilder.put()
.
return false; | ||
} | ||
return isPrintableString(tagValue.value) && | ||
tagValue.value.length <= TAG_KEY_MAX_LENGTH; |
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.
TagValue
doesn't have a length constraint (https://github.com/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md#tagvalue).
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.
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.
I see. We can leave the constraint for now, it's easy to relax it in the future.
@vigneshtdev #268 (comment) FYI, we have decided to add separate package for Tags API. |
@draffensperger thanks for the reviews, I have fixed all the mentioned comments. |
This PR is based on specs : https://github.com/census-instrumentation/opencensus-specs/blob/master/tags/TagMap.md#tagkey