Adds a WithID method to allow setting IDs with names (not just types) #9670
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9670 +/- ##
==========================================
+ Coverage 90.89% 90.92% +0.03%
==========================================
Files 347 347
Lines 18326 18359 +33
==========================================
+ Hits 16657 16693 +36
+ Misses 1344 1342 -2
+ Partials 325 324 -1 ☔ View full report in Codecov by Sentry. |
| @@ -123,19 +123,22 @@ func (t Type) String() string { | |||
| // This must be kept in sync with the regex in cmd/mdatagen/validate.go. | |||
| var typeRegexp = regexp.MustCompile(`^[a-zA-Z][0-9a-zA-Z_]*$`) | |||
|
|
|||
| const NoType = Type("") | |||
There was a problem hiding this comment.
Why do you need this public?
There was a problem hiding this comment.
We don't. We could re-use it downstream or to otherwise identify the "empty" type (the PR on my end which inspired this does a NoType locally), but you're right that this shouldn't be introduced as part of the API by making it public.
(technically my test cases in scraperhelper require this but I could move them to config)
| @@ -151,6 +154,13 @@ func MustNewType(strType string) Type { | |||
| return ty | |||
| } | |||
|
|
|||
| func (t Type) Validate() error { | |||
There was a problem hiding this comment.
@mx-psi do I remember wrongly or you were working to change this to be a struct? So this is not needed since we cannot construct an invalid type.
There was a problem hiding this comment.
Yes, see #9472 which is ready to merge. I think that PR would make this one unnecessary since it would be impossible to construct an invalid type
There was a problem hiding this comment.
Well, technically we still don't allow empty string names for type right? Given the regex does not permit empty strings (also we should probably add a generous length limit imho)
We can move this discussion to a bespoke issue to get some more clarity of thought.
The point w.r.t scraperhelper constructing a nameless ID still exists, unless that's desired. If the current behavior is desired, I'd argue Type would be a better type than ID for the member variable
Regardless, it's probably best to close this PR out in favor of an issue, will comment in 9473, we can spin another out if need be
There was a problem hiding this comment.
Well, technically we still don't allow empty string names for type right? Given the regex does not permit empty strings
I don't think we should allow empty names, at least not through an empty component.Type, but I am not sure what you want to accomplish exactly, so it's better if we discuss in an issue
(also we should probably add a generous length limit imho)
Would you be open to filing an issue about this?
Description:
Optional PR inspired by
-contrib's 31457 I came across 9473I recently had to update a bunch of code related to the
Typeregex lockdown (good thing imo! I'm glad we're doing it!) and wanted some functionality like thiscomponentpackage if you callNewType(one one of theMustfunctions which calls such), but a lot of legacy code directly instantiatesType. While that can always be refactored, I thought having aValidatecommand would be less invasive and allows us to centralize our validation logic. Might want to add it tocomponent.IDas well.scraperwhen the underlying member variable is an ID. IMHO an ideal world would have us either change the type of that member tocomponent.Type, or permit the constructor to take in any validcomponent.ID.Also, as of now we don't length-limit our types, but we may want to start doing that...