Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes after revdep checks of dplyr 0.8.0 RC #223

Merged

Conversation

@romainfrancois
Copy link
Contributor

commented Dec 20, 2018

This fixes two problems that were identified as part of reverse dependency checks of dplyr 0.8.0 release candidate. https://github.com/tidyverse/dplyr/blob/revdep_dplyr_0_8_0_RC/revdep/problems.md#naniar

  • n() must be imported or prefixed like any other function. In the PR, I've changed 1:n() to dplyr::row_number() as naniar seems to prefix all dplyr functions.

  • update_shadow was only restoring the class attributes, changed so that it restores all attributes, this was causing problems when data was a grouped_df. This likely was a problem before too, but dplyr 0.8.0 is stricter about what is a grouped data frame.

romainfrancois added some commits Dec 20, 2018

use dplyr::row_number() instead of 1:n().
`n()`, and `row_number()` no longer get special treatment, and should be imported, or prefixed too, as of dplyr 0.8.0
@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

For reference:

> revdep_details(revdep = "naniar")
══ Reverse dependency check ═══════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════ naniar 0.4.1 ══

Status: BROKEN

── Newly failing

✖ checking examples ... ERROR
✖ checking tests ...

── Before ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
0 errors ✔ | 0 warnings ✔ | 0 notes ✔

── After ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
❯ checking examples ... ERROR
  Running examples in ‘naniar-Ex.R’ failed
  The error most likely occurred in:
  
  > ### Name: gg_miss_case
  > ### Title: Plot the number of missings per case (row)
  > ### Aliases: gg_miss_case
  > 
  > ### ** Examples
  > 
  > 
  > gg_miss_case(airquality)
  Error in n() : could not find function "n"
  Calls: gg_miss_case ... <Anonymous> -> <Anonymous> -> mutate.tbl_df -> mutate_impl
  Execution halted

❯ checking tests ...
  See below...

── Test failures ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── testthat ────

> library(testthat)
> library(naniar)
> 
> test_check("naniar")
── 1. Error: (unknown) (@test-gg-miss-case.R#5)  ───────────────────────────────
could not find function "n"
1: gg_miss_case(airquality) at testthat/test-gg-miss-case.R:5
2: x %>% miss_case_summary(order = TRUE) %>% dplyr::mutate(case = 1:n()) %>% gg_miss_case_create(show_pct = show_pct) at /Users/romain/git/release/dplyr/revdep/checks.noindex/naniar/new/naniar.Rcheck/00_pkg_src/naniar/R/gg-miss-case.R:40
3: withVisible(eval(quote(`_fseq`(`_lhs`)), env, env))
4: eval(quote(`_fseq`(`_lhs`)), env, env)
5: eval(quote(`_fseq`(`_lhs`)), env, env)
6: `_fseq`(`_lhs`)
7: freduce(value, `_function_list`)
8: function_list[[i]](value)
9: dplyr::mutate(., case = 1:n())
10: mutate.tbl_df(., case = 1:n()) at /Users/romain/git/tidyverse/dplyr/R/manip.r:416
11: mutate_impl(.data, dots) at /Users/romain/git/tidyverse/dplyr/R/tbl-df.r:91

── 2. Error: (unknown) (@test-special-missing-values.R#131)  ───────────────────
`.data` is a corrupt grouped_df, the `"groups"` attribute must be a data frame
1: airquality %>% bind_shadow() %>% dplyr::group_by(Month) %>% recode_shadow(Ozone = .where(Wind <= 
       5 ~ "broken_machine")) at testthat/test-special-missing-values.R:131
2: withVisible(eval(quote(`_fseq`(`_lhs`)), env, env))
3: eval(quote(`_fseq`(`_lhs`)), env, env)
4: eval(quote(`_fseq`(`_lhs`)), env, env)
5: `_fseq`(`_lhs`)
6: freduce(value, `_function_list`)
7: withVisible(function_list[[k]](value))
8: function_list[[k]](value)
9: recode_shadow(., Ozone = .where(Wind <= 5 ~ "broken_machine"))
10: data %>% update_shadow(unlist(suffix, use.names = FALSE)) %>% dplyr::mutate(!!!magic_shade_case_when) at /Users/romain/git/release/dplyr/revdep/checks.noindex/naniar/new/naniar.Rcheck/00_pkg_src/naniar/R/shadow-recode.R:235
11: withVisible(eval(quote(`_fseq`(`_lhs`)), env, env))
12: eval(quote(`_fseq`(`_lhs`)), env, env)
13: eval(quote(`_fseq`(`_lhs`)), env, env)
14: `_fseq`(`_lhs`)
15: freduce(value, `_function_list`)
16: withVisible(function_list[[k]](value))
17: function_list[[k]](value)
18: dplyr::mutate(., !!!magic_shade_case_when)
19: mutate.tbl_df(., !!!magic_shade_case_when) at /Users/romain/git/tidyverse/dplyr/R/manip.r:416
20: mutate_impl(.data, dots) at /Users/romain/git/tidyverse/dplyr/R/tbl-df.r:91

══ testthat results  ═══════════════════════════════════════════════════════════
OK: 469 SKIPPED: 15 FAILED: 2
1. Error: (unknown) (@test-gg-miss-case.R#5) 
2. Error: (unknown) (@test-special-missing-values.R#131) 

Error: testthat unit tests failed
Execution halted

2 errors ✖ | 0 warnings ✔ | 0 notes ✔
@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

I believe the error is spurious

@njtierney

This comment has been minimized.

Copy link
Owner

commented Dec 27, 2018

Thanks for this, @romainfrancois ! I'll take a look on my machine and try and work out those errors.

Thanks for finding that issue with class vs attributes - I had sort of equated the two in my head, need to read up on the differences.

@njtierney njtierney changed the base branch from master to revdep-dplyr-080 Dec 27, 2018

@njtierney njtierney merged commit 6bb4134 into njtierney:revdep-dplyr-080 Dec 27, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

In short the class is one attribute

@njtierney

This comment has been minimized.

Copy link
Owner

commented Dec 27, 2018

That actually really helps trigger my understanding of this, I know what I'm looking for to learn more about this now, thanks! :)

@njtierney

This comment has been minimized.

Copy link
Owner

commented Dec 27, 2018

OK so I get no errors on my machine, although I did only submit a patch release a few weeks ago, so I might try and include this in the next release for 0.5.0 around the end of Jan.

njtierney added a commit that referenced this pull request Dec 28, 2018

@romainfrancois romainfrancois deleted the romainfrancois:revdep_dplyr_0.8.0 branch Feb 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.