almartin82 / mapvizieR

visualizations and reports for the NWEA MAP assessment in R
Other
17 stars 6 forks source link

new grouping method #297

Closed almartin82 closed 8 years ago

almartin82 commented 8 years ago

closes #294

almartin82 commented 8 years ago

@chrishaid this comment is sad for us

dplyr group_bys don't preserve the class of the object...

almartin82 commented 8 years ago

@chrishaid these wercker builds claim to be passing, but in reality CMD check is failing...

from the details of the wercker build above:

Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  Loading required package: gridExtra
  Loading required package: magrittr

  Attaching package: 'magrittr'

  The following objects are masked from 'package:testthat':

      equals, is_less_than, not

  Error: 'summary.mapvizieR_cdf' is not an exported object from 'namespace:mapvizieR'
  testthat results ================================================================
  OK: 181 SKIPPED: 0 FAILED: 0
  Execution halted

that's definitely not a pass...

almartin82 commented 8 years ago

@chrishaid been thinking about this for a minute. I see hadley's point - dplyr is a data pipeline package. if you're using it, you're changing the object. in my head, the default behavior would be to preserve the class of the object after transformation, but maybe that's not true for everyone.

trying to think about how to write custom mapvizieR method dispatch per hadley's suggestion. is it just:

that wouldn't be very hard...

almartin82 commented 8 years ago

this is not ready for prime time yet.

chrishaid commented 8 years ago

This is the saddest aborted pull request on mapvizieR ever.

I agree. We should just write 'dplyr verb' methods that dispatch on out classes.

  1. Take object and record classes
  2. Apply dplyr verb to data
  3. Add back saved classes.