Upgrading from 0.4.x to 0.6.0

[eliasbagley]

Hi Hannes - great work on this library.

I just finished migrating from 0.4.1 to 0.6.0, and initially ran into an issue where my inserts and deletes weren't being recorded. I figured out that this is because the insert and delete functions are now async and returning an Observable, and are only recorded when the Observable is subscribed to. I think others with the same problem would be able to track it down much quicker if these functions were annotated with @CheckResult, as they will be notified that they are calling insert and delete, yet not subscribing to the Observable.

What do you think? I can submit a PR if you are in agreement.

[sockeqwe]

Hi,

functions are now async

async is not entirely correct, they are deferred, which means they are executed once subscribed and not during creation of observable as they were before (0.6.0). But yes, you are right that behaviour has changed.

Regarding@CheckResult , I'm not entirely sure if this is the right use case, but to be honest I have never seen @CheckResult before :smile:

The javadoc says:

Denotes that the annotated method returns a result that it typically is an error to ignore.

Well, this may apply on insert() or update() because you should not "ignore" the result, but is it really the same as the example from the java docs:

public @CheckResult String trim(String s) { return s.trim(); 
  ...
  s.trim(); // this is probably an error
  s = s.trim(); // ok
 }

If you shouldn't do anything with the return value (Observable) from Observable insert() then it would rather be void insert(). So I'm not sure if this is really the correct use case. Otherwise we also should annotate the observables retrieved from a SELECT query with @CheckResult or basically every method that returns something. I mean for String.trim() this makes sense because there it isn't that obvious because you are calling it on the same class String.

Furthermore, in RxJava almost no Observable is doing something until it gets subscribed. So you should be familiar with that concept and knowing that you have subscribe the observable ...

What do you think?

[eliasbagley]

I agree that normally a return type is sufficient to indicate that you should do something with the result. But in this case, I think this bit from the Support Annotation docs applies: "You don’t need to go and annotate every single non-void method with this annotation. It’s main purpose is to help with cases where users of the API may be confused and may think that the method has a side effect." http://tools.android.com/tech-docs/support-annotations

I think in the case of this upgrade where a void method changed to a non-void method with no compiler errors yet different runtime behavior, then there is a significant chance of confusion. I knew that SqlBrite 0.6 added stricter semantics around Schedulers, so I assumed that's where my error was and didn't even think to check for a changed return type. It wasn't until I was convinced I was handing the Schedulers correctly that I checked the SqlDao samples and saw how you were subscribing to the result of the insert call.

Either way, I don't think we'd lose anything by adding this annotation for only the cases where the return type behavior changed from void to Observable in the upgrade. The only result of adding @CheckResult to these calls will be that Android Studio will highlight the cases where you're calling these methods and doing nothing with them, which would have alerted me to my error right away.

[sockeqwe]

where a void method changed to a non-void method

This is not true, insert() or update() have always returned Observable even before 0.4.0

Nevertheless, yes it will hurt nobody if we add @CheckResult. Do you want send a pull request?