-
Notifications
You must be signed in to change notification settings - Fork 14
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
treat default of supported_as_column as true in dialect #56
treat default of supported_as_column as true in dialect #56
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
=======================================
Coverage 55.13% 55.13%
=======================================
Files 35 35
Lines 6861 6870 +9
=======================================
+ Hits 3783 3788 +5
- Misses 2885 2887 +2
- Partials 193 195 +2 ☔ View full report in Codecov by Sentry. |
functions/dialect.go
Outdated
} | ||
|
||
type dialectTypeInfo struct { | ||
SqlTypeName string `yaml:"sql_type_name"` | ||
SupportedAsColumn bool `yaml:"supported_as_column"` | ||
SupportedAsColumn *bool `yaml:"supported_as_column" default:"true"` |
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.
let's avoid change to pointer
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.
This is needed for SetDefaults to differentiate the zero value from the provided value.
Without this it is incorrectly setting the value to true even if the input is
supported_as_column: false
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 you resolve this by overriding UnmarshalJSON and having it set the defaults before doing the other operations?
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 tried UnmarshalYAML, one hiccup with this is we cannot use defaults.Set
on any struct using dialectTypeInfo. I had to remove defaults.Set
on dialectFile.
Looks like whenever we have default:"true"
the choice is between 1. using pointer vs 2. adding UnmarshalYAML in all the structs (that includes this bool indirectly) to set the defaults.
LMK 1 or 2?
functions/dialect.go
Outdated
SupportedAsColumn *bool `yaml:"supported_as_column" default:"true"` | ||
} | ||
|
||
func (ti *dialectTypeInfo) isSupportedAsColumn() bool { |
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.
shouldn't need to introduce this if we stay away from pointer.
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.
lgtm
No description provided.