-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: lowercase operators support in the where clause is updated #3657
Conversation
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.
Screen.Recording.2023-10-03.at.2.54.18.PM.mov
@palashgdev please check this
Screen.Recording.2023-10-04.at.11.42.07.AM.mov |
i am still working over this one above issue is still not fixed |
ohh okay |
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.
@palashgdev please create a new issue for supporting update in operators.
Can we please spend some time doing some minimum testing? It reflects very poorly on us. I would revert this immediately. I haven't tried anything complicated. https://www.loom.com/share/68d339549bf140adb18390d8dcd85ded. |
Thanks Srikanth for pointing it out, I mostly checked things related to upercasing and lowercasing. But a lot seems to be missed while testing. I think for critical parts like this unit tests would have helped right ? @palashgdev @YounixM |
Unit tests do help with this and we will be starting to for few flows this week, however, when core functionalities are worked upon, we need to test all the possible flows and then provide signoff. Due to time constraints, we might not be able to add test cases for every possible flow, but we should spend some testing the core functionalities. I agree with @srikanthccv. Let's revert this PR and fix all bugs around this and then merge. |
@YounixM if you could enable more devs to add tests easily, we all can chip-in to cover more test cases |
close #3645