r-hyperspec / hySpc.dplyr

Interface between hyperSpec and dplyr
https://r-hyperspec.github.io/hySpc.dplyr/
MIT License
5 stars 1 forks source link

add dplyr::rename #11

Closed eoduniyi closed 4 years ago

eoduniyi commented 4 years ago

Implemented dplyr::rename with unit test, associated documentation, and vingette explanation.

cbeleites commented 4 years ago

Hi @eoduniyi ,

I just pushed a commit to fix the attributes of the data.frame returned by rename.hyperSpec. I also added a unit test to make sure the attributes are as supposed.

eoduniyi commented 4 years ago

Dear @cbeleites as you may have noticed I'm having some trouble with unittesting and understanding the basics of hyperSpec objects. Also, it appears that Travis CI keeps failing on this branch after I make updates to it...I was wondering if you could explain that. I'm not super familiar with Travis TBH.

Finally, now when I try to load_all() or build my local copy of hyperSpec.tidyverse I get the following error Error: object ‘flu’ is not exported by 'namespace:hyperSpec', which I believe is referencing filter.R. However, I checked the filter.R file and the NAMESPACE file and they both have importFrom hyperSpec flu and importFrom(hyperSpec, flu)?

Screen Shot 2020-03-27 at 9 30 09 PM
eoduniyi commented 4 years ago

Dear @cbeleites,

I have updated rename.R implementation and its associated unit test. I used covr to check how well the test covered rename.R(see img).

On one hand, I have a feeling %100 coverage should be taken with a grain of salt. On the other hand, I am having a hard time coming up with more unit tests atm. So as always, if you have any suggestions that would be awesome.

Also, I took a look at Travis and realized why it kept failing (something related to the examples), fixed them, and so it seems to pass the checks now. Finally, my notes for this test can be viewed here under section 3.

Best, EO

eoduniyi commented 4 years ago

+1

One question to all, though: is hy_update_labels() a good name?

  • Possible confusion with function to actively (by user) set new labels?
  • Would hy_fix_labels() or hy_keep_labels() be more to the point?

I think hy_fix_labels() is more to the point.