Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Remove charset restrictions on tag values #639

Merged

Conversation

semistrict
Copy link
Contributor

Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Great, thank you @Ramonza, just some minor comments but otherwise LGTM!

tag/validate.go Outdated
@@ -52,5 +52,5 @@ func checkValue(v string) bool {
if len(v) > maxKeyLength {
return false
}
return isASCII(v)
return true
Copy link
Member

Choose a reason for hiding this comment

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

In this case we can fold this entirely into

func checkValue(v string) bool {
        return len(v) <= maxKeyLength
}

Insert(k5, "v\x19"),
Upsert(k5, "v\x19"),
Update(k5, "v\x19"),
Insert(k5, strings.Repeat("x", 256)),
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this could be a test of its own since the restriction being newly tested is len(v) <= 255

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added t.Run, so it should appear as it's own test

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks. LGTM!

@semistrict semistrict merged commit 9216254 into census-instrumentation:master Mar 27, 2018
@rakyll rakyll mentioned this pull request Apr 5, 2018
rakyll added a commit to rakyll/opencensus-go that referenced this pull request Apr 5, 2018
rakyll added a commit that referenced this pull request Apr 6, 2018
wingyplus added a commit to wingyplus/opencensus-go that referenced this pull request Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants