[wip] fixes #4335: warn if negative offset was provided

[aghArdeshir]

HI. I started working on this. I faced some challenges I need advice on.

• First of all, I'm only familiar with mysql. Is negative offset invalid for all kind of databases? Should I consider database type?
• Then I found offset function in querybuilder.js file, and extended an existing condition:
  
• The problem with above solution was that negative offset values resulted in a warn log (orange in my case) and were automatically omitted (or converted to zero). I thought this automatic omitting makes bad logic/behavior is user's code hidden or hard-to-debug.
• Later I thought maybe it's not a bad idea if we threw an error in case of negative offset
  
• The above solution can be and not be a breaking change. It can be a breaking change, because it did not use to throw an error, but can not be thought of a breaking change because the query would result to an exception throw (by the database) later anyway.
• The final solution I came up with is this:
  

I thought if it would be a nice idea if we warn user that this behavior (providing negative value to offset) would be deprecated and throw in a future version?

Thanks if you really read all that ♥️

[kibertoad]

@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 (test/integration2) for this?

[aghArdeshir]

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 ?

[kibertoad]

Is it enough to just warn the user that passing negative values to offset will be deprecated in a future version ?

Yup!

[aghArdeshir]

Heay @kibertoad . Nice profile picture 👍

You answered my question about Is it enough to just warn the user that passing negative values to offset will be deprecated in a future version ?. But I read the discussion over and over again here and in the issue (#4335) and decided to revert the functionality to throwing an error. I think its a good practice, as like a bug is fixed.

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

[aghArdeshir]

Is the test and code ok @kibertoad ?

[kibertoad]

@Ardeshir81 Sorry for the delay, will review today.

[kibertoad]

Released in 0.95.3!