refactor: about page

[sphrak]

something more like the andotp about page, which is really nice.

• about app (short description)
• version, license, changelog(!), app source
• libraries used
• actually open links in browser

example

[JanStoltman]

@sphrak I can take care of this. Just let me know where can I get the changelog from.

[sphrak]

@JanStoltman If you feel inclined you can just put a file CHANGELOG in the project root -- and just add a row for your feature branch like;

0.1.0
-----

- added fancy about page

.. or something like that -- until the MVP is done there wont really be any changelog anyway.. or rather it doesnt make sense to do until there is a mvp -- Im thinking when milestone 0.1.0 is done, all features will be added to that version in the changelog file.. then one can start tracking changes between versions better.. unless you have a better idea?

[JanStoltman]

@sphrak Makes sense, I will just create a stub file

[sphrak]

@JanStoltman forgot to ask, will this be a new feature branch different from the current PR? https://github.com/vadret/android/pull/15

[JanStoltman]

@sphrak Yes, I thinks it will be cleaner to create another branch for the complete refactor

[sphrak]

@JanStoltman Superb, I will merge ur PR meanwhile.

Im still new to unittests but I followed your examples more or less but for the AlertRepositoryTest I see failures https://travis-ci.org/vadret/android/builds/432201264 I believe its because I am relying on a default value on the interface (so I dont have to call get() with args) here -- and when I mock the constructor isnt called thus the value will be null and NPE happens and fails the test.. do you have any idea?

(naively I tried passing the ALERT_API_URL to get()` but still causes an NPE)

failing tests;

• https://github.com/vadret/android/blob/feature/alerts/app/src/test/java/fi/kroon/vadret/data/alert/AlertRepositoryTest.kt#L65
• https://github.com/vadret/android/blob/feature/alerts/app/src/test/java/fi/kroon/vadret/data/alert/AlertRepositoryTest.kt#L79

[JanStoltman]

@sphrak
Tests break because of this line

Line 70 in tests breaks because runtime exception is thrown before the subscription happens so neither doOnError nor onErrorReturn catches it Line 84 in tests breaks because map is used on null

At line 70 mockAlertApi should return Single.error(RuntimeException()) At line 84 mockAlertApi shouldn't return null as get it's declared as returning something NonNull (So either make it nullable (But that doesn't make any sense) or just return Single.just(null))

[sphrak]

@JanStoltman thank you for explaining, I do realize my mistake now. A comment on your last sentence there -- how is Single.just(null) different from just null? because that too produces a NPE -- whereas just an empty string does not -- it passes then

[JanStoltman]

@sphrak null is well null, you cannot call any functions on it. Single.just(null) is a single which returns null in it's onNext.

In case of this test it also throws NPE because (I'm guessing, I cannot test it right now) of this it.body()!!as you try to cast null returned by body() to a nonnull Alert

[sphrak]

@JanStoltman maybe im not getting it but when I read here https://github.com/ReactiveX/RxJava/wiki/What's-different-in-2.0#nulls I take it as Single.just(null) just doesnt fly in Rxjava2?

[JanStoltman]

@sphrak If I understand it correctly, returning Single.just(null) will cause a NPE to be propagated downstream so doOnError will be triggered. On the other hand returning null will just cause NPE to be thrown.

Let me check it, I will let you know once I'm sure that I'm not confused/mistaken

[JanStoltman]

@sphrak Ok, I've checked it and it turns out that I was mistaken. Single.just(null) throws and NPE at it's creation time, which makes sense as the parameter of just() is annotated as NonNull.

So in case of null the NPE is thrown in code as one tries to call a method of null, and in case of Single.just(null) the NPE is thrown in test as the just itself throws it when it's parameter is null.

[sphrak]

@JanStoltman Ah okey, thats alright tho -- do you think its okay to just have it return an empty string like this? https://github.com/vadret/android/blob/feature/alerts/app/src/test/java/fi/kroon/vadret/data/alert/AlertRepositoryTest.kt#L77 it does pass, but again im still new on using mockito -- so not entirely sure what happens behind the scene

[JanStoltman]

@sphrak It's not that good of an idea, AlertApi should return Response<Alert> not a String. So this test isn't in the expected value range. It would be better for it to return Response<Alert> will null body.

[sphrak]

@JanStoltman Alright, ill go with a null-body Response then.

I've started to work on a radar view -- but it sucks cuz they have a different base url for that resource -- so one have to send full path URLs which is fine.. but since there is Path variables in that url also it does suck because I think I cant leverage the @Path functionality that way provided by retrofit

• https://opendata-download-radar.smhi.se/api/version/latest/area/sweden/product/comp/{year}/{month}/{date}

[JanStoltman]

@sphrak Why can't you? As far as I know it can be quite nicely done with two BASE_URLs and @Path annotation. What exactly is the problem?

[sphrak]

@JanStoltman Are you sure about that? I couldnt find anything of value it seemed. I am not aware of any way to change the BASE_URL at runtime because its created during the instance creation in the api module (dagger) as per:

fun retrofit(okHttpClient: OkHttpClient, moshi: Moshi): Retrofit {
    return Retrofit.Builder()
            .baseUrl(BASE_API_URL)
            .addCallAdapterFactory(RxJava2CallAdapterFactory.create())
            .addConverterFactory(MoshiConverterFactory.create(moshi))
            .client(okHttpClient)
            .build()
}

where BASE_API_URL is for the forecasts.

• Forecast: https://opendata-download-metfcst.smhi.se
• Radar: https://opendata-download-radar.smhi.se/

However one can send a full path URL by annotating the api method with a @Url to treat it as a full blown url -- this does solve the problem with 2 different base url's -- however this approach becomes problematic if one wants to change parts of the new full Url..

But again I might be mistaken or have missed something so please enlight me

[JanStoltman]

@sphrak There is this library for changing baseurl at runtime. Another option (I use this one) is to create separate ApiService for each url as they are separate data sources after all.

[sphrak]

@JanStoltman I like the latter approach better, Ill just use some qualifiers to differentiate between the instances in dagger and that ought to work :-)

[JanStoltman]

@sphrak Btw, I've noticed that you have provider methods for classes where constructor injection would be enough. Is there any reason for it?

[sphrak]

@JanStoltman There is likely no reason more than me being a total beginner :-) Though this discussion seems like it could need a proper chat instead -- would it be ok to take discussions into a chat instead perhaps?

[JanStoltman]

@sphrak Sure, I guess we got a little off-track here :D

[sphrak]

@JanStoltman alright, I will shoot you an email to the address you have on ur github profile and we'll take it from there.