-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[wip] fixes #4335: warn if negative offset was provided #4361
Conversation
@Ardeshir81 Makes sense to make negative value support deprecated! So what happens now if value is < 0? If I understand the code correctly, it is ignored, which is equivalent to passing 0, which is same is it worked before, right? Could you also add integration test ( |
No it is not the same as it worked before. As stated in issue (#4335), it resulted in querying negative offset on database which threw an error by the db itself (not the library). I believe throwing an error is a good practice, because, in my case it helped me find a bad logic in my code, But automatic ignore/conversion to zero, may make library users' bugs hard-to-find-&-debug. So yes! You got it correctly, it is ignored, which is equivalent to passing 0, but it log warns the user about it. I'll add integration tests 👍 . But what is the deprecation process? Is it enough to just warn the user that passing negative values to offset will be deprecated in a future version ? |
4a8a9f6
to
f30705f
Compare
Yup! |
af92911
to
fde7d94
Compare
fde7d94
to
719cc9d
Compare
Heay @kibertoad . Nice profile picture 👍 You answered my question about I squashed the commit. And also wrote tests. I had problems understanding and running tests, so wrote one in a different style. is it ok ? thanks |
Is the test and code ok @kibertoad ? |
@Ardeshir81 Sorry for the delay, will review today. |
Released in 0.95.3! |
HI. I started working on this. I faced some challenges I need advice on.
mysql
. Is negative offset invalid for all kind of databases? Should I consider database type?offset
function inquerybuilder.js
file, and extended an existing condition:zero
). I thought this automatic omitting makes bad logic/behavior is user's code hidden or hard-to-debug.I thought if it would be a nice idea if we warn user that this behavior (providing
negative
value tooffset
) would be deprecated and throw in a future version?Thanks if you really read all that♥️