refactor: Unify Field Kind and Schema properties #2414
Conversation
AndrewSisley
added
area/schema
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2414 +/- ##
===========================================
- Coverage 74.93% 74.91% -0.02%
===========================================
Files 268 268
Lines 25856 25888 +32
===========================================
+ Hits 19373 19393 +20
- Misses 5157 5172 +15
+ Partials 1326 1323 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| // Underlying returns the unterlying Kind as a string. | ||
| // | ||
| // If this is an array, it will return the element kind, else it will return the same as | ||
| // [String()]. |
There was a problem hiding this comment.
thought: I think that this notation is used in some other places as well but here it threw me off given the context. I was wondering why calling Undelying() on a non array would return String() wrapped in an array.
There was a problem hiding this comment.
Square brackets are how to correctly link stuff in Go docs, although I'm not really sure it works for funcs. I used to use backticks as that feels more normal (and is what rust uses), but the Go devs went with square brackets in 1.19:
| case uint8(FieldKind_BOOL_ARRAY), uint8(FieldKind_INT_ARRAY), uint8(FieldKind_FLOAT_ARRAY), | ||
| uint8(FieldKind_STRING_ARRAY), uint8(FieldKind_NILLABLE_BOOL_ARRAY), uint8(FieldKind_NILLABLE_INT_ARRAY), | ||
| uint8(FieldKind_NILLABLE_FLOAT_ARRAY), uint8(FieldKind_NILLABLE_STRING_ARRAY): | ||
| f.Kind = ScalarArrayKind(intKind) | ||
| default: | ||
| f.Kind = ScalarKind(intKind) |
There was a problem hiding this comment.
thought: This as the potential to be quite annoying to maintain as we update out list of supported array types. I'm wondering if we should take the hit now and change the assigned number ranges to something like:
- [0:100] -> Scalar types
- [100:200] -> Array types
We wont be using the full range but at least if we ever need to do another switch statement it would be a much cleaner check.
There was a problem hiding this comment.
I dislike the current too, however I don't like the range option, and would probably prefer a custom encoding: First bit for nillable, second bit for array, then the scalar id.
As a proper solution would require a bit of thought and an agreed design, I'd prefer if this conversation was out of scope here.
Schema was removed from the path a long time ago, correct error being returned was a coincidence.
The upcoming changes to FieldKind will prevent == checks like this. That IsObject also returns true for arrays of objects is a bit weird in my opinion, but changing that is out of scope.
Zero will not be a valid value with the upcoming change.
This is dead code and has been for quite a long time. Maintaining it with the upcoming change will be a pain.
AndrewSisley
force-pushed
the
2409-unify-field-kind-schema
branch
from
aae93ac
to
f32d759
Compare
Relevant issue(s)
Resolves #2409
Description
Unifies the Field Kind and Schema properties.
Commits should be clean, although most of the work is in the last. Tests have been modified/deleted in other ways besides cid changes as this PR changes public behaviour (heads up in case the first few files make it look like the usual cid change stuff).