Breaking changes in next CRAN update

[Tazinho]

Hi Daniel, hi Sam (@strengejacke , @sfirke)

in the next CRAN update will be breaking changes.

These changes are relatively small for you, but I guess that at least the 2nd one is very important for the simplification of the snakecase pkg.

Particularly these changes include:

1. Rename the argument parsingoption in parsing_option
2. The internal implementation of the "protect process" will change:
   Therefore I will also

• Softly deprecate protect, set protect = "_(?![:alnum:])|(?<![:alnum:])_" as the new default and remove this argument later from the package. (I tend to use this option to make a smoother change)
• Or I might just set this internally as the default and remove protect directly from the interface.

Practically his means, that "_" will be removed when it is around a non alpha-numeric character. For example there won't be things like "sepal_._length" or "malte_._grosser_@_gmail_._com" as artifacts in the output anymore. They will be just "sepal.length" or "malte.grosser@gmail.com" (if neither "\\." nor "@" is in the preprocess argument).
Internally this happens before the "postprocessing" starts so these artifacts also won't occur, if anything else than "_" is supplied to the postprocess argument.

For example

to_any_case("hamburg-city@meetup_1.1.18", case = "snake", preproces = "-|@")
##> "hamburg_city_meetup_1.1.18"
to_any_case("hamburg-city@meetup_1.1.18", case = "snake", preproces = "-|@", postprocess = ">")
##> "hamburg>city>meetup>1.1.18"

Therefore also behaviour in the janitor::clean_names() function will be affected. However, the implementation can stay the same, as neither protect nor parsingoption are explicitly called in the code. In my opinion, this new behaviour is far better.

JANITOR
However, two of the tests in janitor will fail after the update, so I will change these tests on the snakecase side and I recommend to do this also on the janitor side:

SJLABELLED
As far as I know the protect option is used within sjlabelled here.
I would recommend to remove this option after the snakecase update from the code. I am not sure if the snakecase function is used anywhere else in the sj-universe, but of course there should apply the same.

If you have any issues regarding these updates or will have any user feedback/critics in the future about this, pls let me know.

Cheers,
Malte

[Tazinho]

Basically everything one needs to know is now in the basic examples
https://github.com/Tazinho/snakecase

If you don't see any serious issues and are willing to update your packages, I will go very soon to the master branch with this update and also make a CRAN submission relatively quickly.

[sfirke]

So thoughtful of you to create this issue. I installed the dev version of snakecase and read the README, the thought that you've put into this is quite impressive! It was fun to learn more about the package.

I will summarize my understanding to see if I get this:

I reviewed the changes you made to your tests and verified on my own and it looks like the changes to the janitor test outputs are actually making the lower_upper and upper_lower treatment of period-separated numbers consistent with how they're treated in the other case arguments. That is, 1.2.3 becomes 1_2_3 instead of the previous 123. That seems like a clearly good thing and I can cheerfully update the tests you identified when this goes to CRAN.

Will there be other changes to the behavior of clean_names() that I haven't accounted for in the tests? I'm trying to think through that. Since I'm feeding in the result of a call to make.names() my input to to_any_case will only ever be letters, numbers, and .. And I call to_any_case() with the preprocess argument of "\\." so it will be treated as a separator. I can't think of anything else, this change to snakecase seems to make clean_names() slightly more consistent.

[sfirke]

Regarding timing, feel free to move as fast as you like on this. I'm hoping to submit the version of janitor that depends on snakecase to CRAN around the end of the month. If changes to to_any_case behavior are already out there before the big janitor update, all the better.

[Tazinho]

Thanks also for looking into this. I am really happy about this new behaviour and that it also Sounds reasonable to you.

If I (or @strengejacke) don‘t encounter any bugs, the last commits will be regarding the consistent behaviour between upper/lower camel and lower_upper, upper_lower. Everything else is just technical, like issues about pkgdown or testcoverage...and of course looking a last time into clean_names implementation. Then CRAN :) and closing here.

[strengejacke]

Or I might just set this internally as the default and remove protect directly from the interface.

If you choose this option, then sjlabelled needs to be submitted before snakecase, else revdep checks will fail.

[Tazinho]

Or I might just set this internally as the default and remove protect directly from the interface.

If you choose this option, then sjlabelled needs to be submitted before snakecase, else revdep checks will fail.

Good point. So it will be deprecated.

[Tazinho]

There is another very small (and positive) breaking change in clean_names(). When I look closer at this, there might be even room for more improvement for this case. But at this we should look later on the janitor side I think.

[Tazinho]

EDIT: Problem is already solved and warning is gone (now idea why). Will submit tomorrow.

@strengejacke I am getting a strange warning when running revedev_check() for your ggeffects. Maybe you know what it is about, how to solve it or if it is important? I am currently tending to ignore it and just proceed!? Any thoughts?

> checks
$revdeps
NULL

$platform
 setting  value                       
 version  R version 3.4.3 (2017-11-30)
 system   x86_64, mingw32             
 ui       RStudio (1.1.383)           
 language (EN)                        
 collate  German_Germany.1252         
 tz       Europe/Berlin               
 date     2018-01-08                  

$dependencies
 package   * version date       source                           
 covr        3.0.1   2017-11-07 CRAN (R 3.4.3)                   
 knitr       1.18    2017-12-27 CRAN (R 3.4.3)                   
 magrittr    1.5     2014-11-22 CRAN (R 3.4.3)                   
 purrr       0.2.4   2017-10-18 CRAN (R 3.4.3)                   
 purrrlyr    0.0.2   2017-05-13 CRAN (R 3.4.3)                   
 rmarkdown   1.8     2017-11-17 CRAN (R 3.4.3)                   
 snakecase * 0.7.1   2018-01-07 local (Tazinho/snakecase@6dbedd0)
 stringi     1.1.6   2017-11-17 CRAN (R 3.4.2)                   
 stringr     1.2.0   2017-02-18 CRAN (R 3.4.3)                   
 tibble      1.4.1   2017-12-25 CRAN (R 3.4.3)                   

$results
$results$ggeffects
## ggeffects (0.3.0)
Maintainer: Daniel Lüdecke <d.luedecke@uke.de>  
Bug reports: https://github.com/strengejacke/ggeffects/issues

0 errors | 1 warning  | 0 notes

checking Rd cross-references ... WARNING
Fehlendes Paket um Rd Kreuzreferenzen zu überprüfen: 'ordinal'

$results$sjlabelled
## sjlabelled (1.0.5)
Maintainer: Daniel Lüdecke <d.luedecke@uke.de>  
Bug reports: https://github.com/strengejacke/sjlabelled/issues

0 errors | 0 warnings | 0 notes

I also tried to fix it via specifying the libpath option for revdep_check() and copying the ordinal pkg binaries into it manually. However, ran into the next error and don't see an intuitive way to proceed here.

> devtools::revdep_check()
Reverse dependency checks for snakecase =========================================
Saving check results in `revdep/checks/`
Saving install results in `revdep/install/`
Computing reverse dependencies... 
Installing dependencies for snakecase to revdep_library
Installing 1 package: testthat
Warning: unable to access index for repository https://bioconductor.org/packages/3.4/bioc/bin/windows/contrib/3.4:
  cannot open URL 'https://bioconductor.org/packages/3.4/bioc/bin/windows/contrib/3.4/PACKAGES'
Warning: unable to access index for repository https://bioconductor.org/packages/3.4/data/experiment/bin/windows/contrib/3.4:
  cannot open URL 'https://bioconductor.org/packages/3.4/data/experiment/bin/windows/contrib/3.4/PACKAGES'
Warning: unable to access index for repository https://bioconductor.org/packages/3.4/extra/bin/windows/contrib/3.4:
  cannot open URL 'https://bioconductor.org/packages/3.4/extra/bin/windows/contrib/3.4/PACKAGES'
Warning: package ‘testthat’ is in use and will not be installed
Installing snakecase 0.7.1 to C:\Users\MGO\AppData\Local\Temp\RtmpOOHyD9\revdep4e4c20306abc
Setting env vars ----------------------------------------------------------------
NOT_CRAN    : false
RGL_USE_NULL: true
DISPLAY     : 
Checking 2 CRAN packages ========================================================
Results saved in D:\R\Projects\snakecase/revdep/checks
Package library: D:/R/Projects/snakecase/revdep_library, C:/Users/MGO/AppData/Local/Temp/RtmpOOHyD9/revdep4e4c20306abc, D:/R/Projects/snakecase/packrat/lib-R
Installing dependencies ---------------------------------------------------------
Checking packages ---------------------------------------------------------------
Checking 2 packages: ggeffects, sjlabelled
Package library: C:/Users/MGO/AppData/Local/Temp/RtmpOOHyD9/revdep4e4c20306abc, D:/R/Projects/snakecase/revdep_library, D:/R/Projects/snakecase/packrat/lib-R
Error: Check failed: 'D:\R\Projects\snakecase\revdep\checks/ggeffects.Rcheck' doesn't exist

[strengejacke]

Kreuzreferenzen

I think you should use English locale - when I read "Kreuzreferenzen", I personally would also throw a warning, or even an error. ;-)

[Tazinho]

Pls make sure to call variables by name and not by position, since I am relatively sure that I will change the order of the arguments in one of the next updates in a few months. See #117 ...Just to make sure...
It would be a pitty if that would break any dependencies...

[strengejacke]

Except for the first argument, I usually always call arguments by name, just for this reason.

[Tazinho]

@strengejacke I am not sure where this fits best. However, since whitespaces around non-alphanumerics will be suppressed, you might decide to introduce whitespaces via backreferences (regex) to regarding symbols when it makes sense. For example spaces might be useful around „&“ before „(„ and after „)“ and „:“. Just a thought....

[Tazinho]

Just to notify @sfirke and @strengejacke: the next CRAN update (v 0.9) will come when you give me the go (latest in 2 weeks). The update is currently on devversion-01 branch.

Changes:

• parsing_option 5 and 6 are now 3 and 4. The old 3 and 4 are removed.
• purrr + magrittr dependencies have been resolved
• all deprecated (renamed) argument names have been removed
  • string
  • abbreviations
  • preprocess -> sep_in
  • parsing_option
  • replace_special_characters -> transliterations
  • case
  • postprocess -> sep_out

Afterwards, I will only work on small fixes of the documentation and stuff regarding user feedback. This should then end in v1.0.0 in the next months.

[strengejacke]

Just let me say that CRAN team is happy for release cycles with not more than one update per 1-2 months 😉

[sfirke]

Thanks for the heads up. When will you move from devversion-01 to master branch on GitHub for this version? I'll start working from v0.9 and get ready for it.

[Tazinho]

I will switch to master before going to CRAN (in 1 or 2 month now ;-). In this way master and CRAN docu will be in Sync. Or would you prefer if I directly switch to master branch now?

[sfirke]

My approach has been to move development features to master branch when they're stable, well before submitting to CRAN, so that people are more likely to test them and give feedback. I don't know if that's standard best practice or not.

…

On Mon, Jan 29, 2018 at 3:03 PM, Tazinho ***@***.***> wrote: I will switch to master before going to CRAN (in 1 or 2 month now ;-). In this way master and CRAN docu will be in Sync. Or would you prefer if I directly switch to master branch now? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#106 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHOBkNO7VLYLprNCMXzhyCJCkY_qie9Xks5tPiP5gaJpZM4RSLxM> .

[Tazinho]

I think this is the optimal approach. It just felt a bit strange to me to have very small and subtile breaking changes (like changing parsing_option) in the devversion. However, to reduce ambiguity, I will switch to master and try to follow this practice more strictly from now on.

[strengejacke]

Just to be sure: this should work in a future update of snakecase w/o problems?

convert_case <- function(lab, case = NULL, ...) {

  if (!is.null(case) && !is.null(lab)) {

    # set defaults

    prep <- "(?<!\\d)\\."
    posp <- " "


    # check additional arguments

    add.args <- lapply(match.call(expand.dots = F)$`...`, function(x) x)

    if ("preprocess" %in% names(add.args)) prep <- eval(add.args[["preprocess"]])
    if ("postprocess" %in% names(add.args)) posp <- eval(add.args[["postprocess"]])

    if ("sep_in" %in% names(add.args)) prep <- eval(add.args[["sep_in"]])
    if ("sep_out" %in% names(add.args)) posp <- eval(add.args[["sep_out"]])


    # convert

    snakecase::to_any_case(
      lab,
      case = case,
      sep_in = prep,
      sep_out = posp
    )

  } else {
    lab
  }
}

[Tazinho]

Works. Here also a quick check

> library(snakecase)
> 
> convert_case <- function(lab, case = NULL, ...) {
+   
+   if (!is.null(case) && !is.null(lab)) {
+     
+     # set defaults
+     
+     prep <- "(?<!\\d)\\."
+     posp <- " "
+     
+     
+     # check additional arguments
+     
+     add.args <- lapply(match.call(expand.dots = F)$`...`, function(x) x)
+     
+     if ("preprocess" %in% names(add.args)) prep <- eval(add.args[["preprocess"]])
+     if ("postprocess" %in% names(add.args)) posp <- eval(add.args[["postprocess"]])
+     
+     if ("sep_in" %in% names(add.args)) prep <- eval(add.args[["sep_in"]])
+     if ("sep_out" %in% names(add.args)) posp <- eval(add.args[["sep_out"]])
+     
+     
+     # convert
+     
+     snakecase::to_any_case(
+       lab,
+       case = case,
+       sep_in = prep,
+       sep_out = posp
+     )
+     
+   } else {
+     lab
+   }
+ }
> 
> convert_case(names(iris), case = "parsed")
[1] "Sepal Length" "Sepal Width"  "Petal Length" "Petal Width"  "Species"  

[strengejacke]

Yes, it currently works. I was just wondering if it will work with your future plans for snakecase, because I want to submit my update the next days and would like to avoid another submission just some days later just because I forgot something related to forthcoming snakecase changes. ;-)

[Tazinho]

Yes, it will work. I will wait a longer time now to see if there should be changes regarding the interface, but I currently think that the package is stable and there won't be any within the next months for sure.

[strengejacke]

Ok, sjlabelled update is on CRAN, so you can go ahead from my site.

[sfirke]

hi @Tazinho, I merged into janitor/master my branch that relies on the dev version of snakecase, and updated my .travis.yml file to use the dev version of snakecase from github so that my tests pass.

I'm done changing things that rely on snakecase and my ideal process would be for the latest snakecase changes above (where parsing option 4 is the old parsing option 6) to go to CRAN before janitor 1.0. So anytime you want to submit is great with me if you're ready.

Parsing option 4 was great, I wrote some regexes and didn't end up using them 😃 That option did exactly what I wanted to and it would have been hard for me to implement without snakecase.

[sfirke]

Malte, when do you think this version of snakecase with parsing option 4 being the old parsing option 6 will go to CRAN?

[Tazinho]

I can go when you want. But I also wanted to ask you and @strengejacke if you have a better idea for the parsing_option Argument. I think it looks strange for the user and it’s the only part of the pkg that I am not happy about...

[sfirke]

Maybe more descriptive variable name then parsing_option? And the numbers don't have meaning themselves, so if you could describe them with a brief string instead, that might be preferable.

[Tazinho]

That would definitely be an upgrade to the current way. I was also thinking to split the current parsing options into 3 different argumentes like: Parsing_logic (1 vs 2), Conversion_logic (1 vs 3), Numerals_logic (1 vs 4) with named options, which would make it easier to specify an arbitraty combination, but more verbose than the other option...

I am going to wait, to see if more issues regarding parsing, conversion, numerals etc. will come up and try to find a good solution then. However, here should be closed for a relatively long period now :)

Thanks to both of you for your patience...and have a nice weekend.
Update is on its way....

[sfirke]

I see it's on CRAN now! 🎉