Some comments ... (I haven't looked at this that closely before so there are some things here that relate to much earlier commits).
- Logging seems to be the only android specific thing in the library ATM. I'm thinking that maybe we could treat it as a generic java client library using slf4j maybe, and provide pluggability to wire it up to android.util.Log on Android.
I don't know. I need to think about this.
- I think Matthias noticed that the API is nicely aligned with the one for iOS, which is exactly what we want. (I didn't compare, so I can't say :)
- Looking at HttpRestProvider get() method doesn't take an id - it's an operation that retrieves a list of all instances - presuming URL points to something like http://host/task, and not to something like http://host/task/100.
(https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/core/HttpRestProvider.java#L46)
That makes sense in the HTTP meaning of GET.
It means, that if we want to get a single instance of a resource (i.e. Task#100) we have to create a new HttpRestProvider with a new URL.
Make sense. Addind get(id)
For delete(), OTOH we can execute it with a specific id, and the same goes for put() to perform an update of a child resource.
So we have GET, and POST operations performed on a 'parent' resource, and DELETE, and PUT operations performed on child resources. This asymmetry is confusing to me ... non-intuitive.
Me too, I did it just because we do not have a way to find the id dynamically yet
- I wouldn't eat exceptions, and return null even in a not-yet-real exception handling :)
(https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/core/HttpRestProvider.java#L86)
Really, either don't catch it, or if you do catch it, rethrow it as is, or wrap into another - normally RuntimeException is perfectly fine, so you don't pollute your API with throws declarations.
I vote to throw RuntimeException
- I would use IllegalArgumentException here:
https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/AdapterFactory.java#L34
It's more appropriate, as it's a standard pattern for this kind of use-case. By convention UnsupportedOperationException is used for empty methods where interface contract is not fully supported.
Ok!
- We have to do something about this "getId"
(https://github.com/danielpassos/aerogear-android/blob/new-pipeline/src/main/java/org/aerogear/android/pipeline/RestAdapter.java#L76)
One idea is to have an annotation - @Id. But scanning for annotations needs to be done at init time or lazily on first use ...
So maybe we could have an abstract base class with abstract method getId() that every data object has to extend, and implement. A simpler, and more robust solution actually, as compiler will enforce it so there is no way for not providing one, as could happen with forgotten or wrong placement of @Id annotation for example.
That's about it for now ... :)
- marko
----- Original Message -----
>
>
> _______________________________________________
_______________________________________________