Closed nuest closed 6 years ago
@noerw Also added myself to the list of package contributors, hope you agree that is justified by the contributions.
I'm done from my side... tests work on Travis and Appveyor, you only must activate this repo on both platforms. Not sure how the integration with codecov works best... I'd like to look into that after a merge. It might simply be better to call covr::package_..
to get the coverage within the test logs, but the visual reports are nice:
https://codecov.io/gh/nuest/opensensmapR/tree/d82b538b7a7a0a1b593657a073eaef8f888ca6b0/R
Wow, thank you for all this work!
Minor remarks:
skip_on_cran()
[1] for all tests that make a call to the API: [2 (search for skip_on_cran
)]. added this in 7e7b9bc
lintR
config soon. (#20)[.sensebox
implementation. Overriding data.frame functions is an antipattern anyway, I'm considering to use tibbles as a base instead. (#19)I will set up the CI services in the next days, and merge once that's done.
Thanks for merging and the feedback.
About skip_on_cran
: sure, we can do that, #21
About the code style: Yes, but mine's better ;-). Just kidding, happy to update this once you've configured a linter.
About the data.frame
overrides: I don't think it's such a bad practice, is it? I think sf
does it too. Maybe it's simply about not putting the sensebox
class first?
When fixing failing tests I noticed that appveyor does not build vignettes (https://github.com/krlmlr/r-appveyor/issues/83), and seems to incorrectly run tests:
* checking for unstated dependencies in 'tests' ... OK
* checking tests ...
Running 'testthat.R' <-- should load all the other tests & eventually fail?
OK
* DONE
@nuest Do you know how to fix the latter issue?
IMHO the vignettes not being build is not an issue. Appveyor is there to make sure things run on Windows, and the tests take care of that.
Re. the tests: The tests are run and complete successfully, so the package log just has an OK
. That's expected. For details you must take a look at the build artifacts, file testthat.Rout
:
> library("testthat")
> library("opensensmapr")
> library("sf")
Linking to GEOS 3.6.1, GDAL 2.2.0, proj.4 4.9.3
>
> test_check("opensensmapr")
== testthat results ===========================================================
OK: 9 SKIPPED: 26 FAILED: 0
>
> proc.time()
user system elapsed
4.76 0.21 6.96
Only 9 tests are OK, the rest is skipped, which seems to me like the API is not available during the tests... OR the skip_on_cran()
takes effect, because in an older configuration the result was OK: 42 SKIPPED: 1
(also seems like the overall number of tests decreased since then...).
PR is not ready for merge, but feedback is welcome. Once completed this closes #16 (if Travis and Appveyor are activated for the main repo).
CI
Travis and Appveyor tests run for my fork (but not all tests work yet), see
Tests
@noerw Can you take a look why a coercion to sf looses the attribute names? See https://github.com/nuest/opensensmapR/blob/master/tests/testthat/test_box.R#L13
Open tasks
osem_measurements
@nuest