-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
#3939 Acceptable boolean syntax differs for data writes and data queries #4160
Conversation
Thanks @apriendeau -- have you signed the CLA? Also, the commit messages are not very clear. Can you squash the two commits, and include a clearer commit message? |
@@ -10,6 +10,7 @@ | |||
- [#4111](https://github.com/influxdb/influxdb/pull/4111): Update pre-commit hook for go vet composites | |||
- [#4136](https://github.com/influxdb/influxdb/pull/4136): Return an error-on-write if target retention policy does not exist. Thanks for the report @ymettier | |||
- [#4124](https://github.com/influxdb/influxdb/issues/4124): Missing defer/recover/panic idiom in HTTPD service | |||
- [#3939](https://github.com/influxdb/influxdb/issues/3939): [0.9.3] Acceptable boolean syntax differs for data writes and data queries |
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.
Please give yourself credit here Thanks @apriendeau
is what we like to write.
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.
you want me to thank myself? lol
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.
If you don't, we will. :-)
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.
okay, I appreciate it.
I like the idea of symmetry, so I am OK with this. +1 |
@otoolep edit squashed, reworded to be more clear and signed the CLA |
I don't like having |
I don't have a strong opinion, you raise a fair point @dgnorton |
I'm not a fan of |
I am not a fan of 't/f' as keywords but isn't it implied when you can use it in a write? So if we remove it, I would suggest that we only have
I am in favor of removing |
@apriendeau i think it might be good to also have some coverage of this in an integration test to demonstrate that it's correct all the way through the system. also, i think @corylanou's suggestion to do #4037 alongside this is a good idea. |
@apriendeau That's a good point. I think I'd personally rather see |
@toddboom @corylanou Which way would you guys lean towards for acceptable write/read for booleans? If we are looking towards the other route, which is to remove support for t/f in the write query, I will probably close this PR and opened a new one. |
So I made the assumption that
T
andF
are acceptable boolean values but I am not sure where the stance is on supporting that including all the variations of thetrue/false
but if that is the case and we do decide to go forward with this, there are tests to cover all the variations oftrue/false
. So I guess, I am asking which direction we would like to go.