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

Bioc-friendly style feature suggestion #636

Closed
lcolladotor opened this issue Apr 17, 2020 · 2 comments
Closed

Bioc-friendly style feature suggestion #636

lcolladotor opened this issue Apr 17, 2020 · 2 comments

Comments

@lcolladotor
Copy link

Hi,

Based on my recent work for r-lib/actions#84 and at https://github.com/leekgroup/derfinderPlot/blob/master/dev/02_update.R, I want to make a few PR to styler.

When you submit a new package to Bioconductor, they run BiocCheck as in Bioconductor/Contributions#1393 (comment) which links to http://bioconductor.org/spb_reports/regutools_buildreport_20200414095652.html. BiocCheck runs some code style checks and any failures are reported as NOTEs, more on this at http://bioconductor.org/packages/release/bioc/vignettes/BiocCheck/inst/doc/BiocCheck.html#formatting-checks. On that paragraph in the vignette they refer readers to formatR (also on logs when you get an error). However, formatR doesn't work that well as styler (I had issues where say overlaps = "5' prime" gets re-formatted into overlaps = ''5 prime'). There's still the issue about line lengths #247 which BiocCheck will check, but as noted in the issue thread, even formatR can't fix things.

I only discovered styler yesterday and saw that it worked fairly well for my derfinderPlot package. One difference between the tidyverse style guide https://style.tidyverse.org/ and the Bioconductor style guide http://www.bioconductor.org/developers/how-to/coding-style/ that is easy to address, is the 2 vs 4 space indentation. One that is trickier is the definition of functions with many arguments where the tidyverse style recommends this https://style.tidyverse.org/functions.html#long-lines-1 and the bioc style wants 4 spaces still.

With the help of a few print() statements, I looked at tidyverse_style()$indentation$update_indentation_ref_fun_dec and modified it at https://github.com/leekgroup/derfinderPlot/blob/master/dev/02_update.R#L2-L12.

bioc_style <- styler::tidyverse_style(indent_by = 4)
bioc_style$indention$update_indention_ref_fun_dec <- function(pd_nested) {
    if (pd_nested$token[1] == "FUNCTION") {
        seq <- rlang::seq2(3, nrow(pd_nested) - 2)
        pd_nested$indention_ref_pos_id[seq] <- pd_nested$pos_id[nrow(pd_nested)]
        pd_nested$indent[seq] <- pd_nested$indent[seq] + 4
    }
    pd_nested
}

The idea of the potential PR would be to add a function bioc_style() that builds upon tidyverse_style() to make styler more bioc-friendly. It won't be the full bioc-style, but it'll resemble it a lot more. In the future maybe other things I missed that could be added to this function. The goal would be for have Bioc developers run styler::style_pkg(transformers = styler::bioc_style() ) after the PR is complete. Hm... maybe bioc_like_style() would be a better name so it's clearer that it's not 100% the bioc style.

I'll make a companion issue at BiocCheck for updating the recommendation to use styler instead of formatR (or maybe list both). Though having a bioc_style() in styler would be a lot more easy to use than telling them about the code in https://github.com/leekgroup/derfinderPlot/blob/master/dev/02_update.R#L2-L12 or incorporating it in another package.

Best,
Leo

@lorenzwalthert
Copy link
Collaborator

Thanks @lcolladotor. If you had the time to read #340, in particular #340 (comment), you may notice that we are very conservative when it comes to adding new style guides to styler. In #340 (comment), I mention a few options for style guides that are not part or {styler}. We'll have to wrap all of this into formal documentation and hopefully provide concrete instructions. For now, what I can add is:

  • we do not support style guides in this repository that are (very) particular, like the mlr style guide. I guess the Bioconductor community would be large enough to make this argument irrelevant.
  • we do not support style guides that one can easy pack in a package himself by wrapping tidyverse_style and modify some transformer functions that don't depend on styler internals. This seems to be the case for your style guide.

I suggest you fork the example of the oneliner package, rename it, and implement your style there. As you demonstrated above, the package won't be a lot of code (at least not now). I can contribute a few tests in your repo so you can use the styler testing infrastructure (we could export that in a next release). I can also review the changes you make in your oneliner fork once you got started. I am also happy to assist with any other problem. I think it's easier for you and us if you develop the style guide independently of styler, make sure it's tested appropriately and releasing in on CRAN / Bioconductor if needed. styler has grown considerably over time and I'd prefer to keep things modular if possible. I hope that's ok for you.

lcolladotor added a commit to lcolladotor/biocthis that referenced this issue May 9, 2020
Created both an introductory and a developer's notes vignette, updated README
and docs with examples, added a second biocViews term, fixed some small
bugs/typos.

Related links (as many as I could remember):

* https://rstd.io/tidytools19
* https://twitter.com/CVWickham
* https://twitter.com/hadleywickham
* https://www.rstudio.com/products/rstudio/download
* https://comunidadbioinfo.github.io/post/building-tidy-tools-cdsb-runconf-2019/#.XrbLMxNKiu4
* http://bioconductor.org/
* https://lcolladotor.github.io/pkgs/
* https://stat.ethz.ch/pipermail/bioc-devel/2020-March/016365.html
* https://www.bioconductor.org/help/docker/
* https://stat.ethz.ch/pipermail/bioc-devel/2020-April/016532.html
* https://github.com/features/actions
* https://stat.ethz.ch/pipermail/bioc-devel/2020-April/016650.html
* r-lib/actions#84
* r-lib/usethis#1108
* r-lib/styler#636
* Bioconductor/BiocCheck#57
* Bioconductor/bioconductor.org#54
* http://bioconductor.org/developers/how-to/coding-style/
* https://style.tidyverse.org/
* https://twitter.com/lorenzwalthert
* https://twitter.com/mt_morgan
* https://docs.travis-ci.com/user/languages/r/
* r-lib/pkgdown#1206
* r-lib/pkgdown#1230
* https://twitter.com/jimhester_
* https://www.jimhester.com/talk/2020-rsc-github-actions/
* https://github.com/Bioconductor/BBS
* https://github.com/Bioconductor/packagebuilder
* https://www.appveyor.com/
* r-hub/rhub#52
* r-hub/rhub#38
* https://www.tidyverse.org/blog/2020/04/usethis-1-6-0/
* https://github.com/r-lib/actions/tree/master/examples
* https://yihui.org/en/2018/03/second-pull-request/
* https://github.com/r-lib/actions/blob/master/examples/check-standard.yaml
* https://help.github.com/en/actions
* https://ropenscilabs.github.io/actions_sandbox/
* https://twitter.com/seandavis12
* https://github.com/seandavi/BiocActions/blob/master/.github/workflows/main.yml
* https://twitter.com/CSoneson
* https://github.com/csoneson/dreval/blob/master/.github/workflows/R-CMD-check.yaml
* https://bioc-community.herokuapp.com/
* https://github.com/leekgroup/derfinderPlot/blob/master/.github/workflows/check-bioc.yml
* https://github.com/LieberInstitute/recount3/blob/master/.github/workflows/check-bioc.yml
* https://github.com/hpages
* r-lib/actions#68
* r-lib/actions#85
* https://twitter.com/opencpu
* https://community.rstudio.com/u/const-ae
* https://community.rstudio.com/t/compiler-support-fo-c-14-features-on-windows/57284/4
* r-lib/xml2#296
* r-lib/xml2#302
* https://github.com/r-lib/usethis/blob/master/.github/workflows/R-CMD-check.yaml
* https://github.com/r-lib/usethis/commits/master/.github/workflows/R-CMD-check.yaml
* https://stat.ethz.ch/pipermail/bioc-devel/2020-April/016703.html
* https://stat.ethz.ch/pipermail/bioc-devel/2020-April/thread.html
* r-lib/remotes#296
* r-lib/actions#86
* r-lib/covr#427
* https://github.com/r-lib/actions/blob/master/examples/pr-commands.yaml
* https://www.digitalocean.com/community/tutorials/how-to-install-git-on-ubuntu-18-04
* r-lib/actions#50
* actions/checkout#238
* https://github.com/rocker-org/rocker-versioned2/blob/master/dockerfiles/Dockerfile_rstudio_4.0.0-ubuntu18.04
* https://twitter.com/niteshturaga
* https://twitter.com/cboettig
* rocker-org/rocker-versioned#208
* https://github.community/t5/GitHub-Actions/bd-p/actions
* https://www.r-consortium.org/blog/2020/03/18/cdsb-diversity-and-outreach-hotspot-in-mexico
* https://github.com/maxheld83
* r-lib/actions#87
* https://github.com/yutannihilation
@lcolladotor
Copy link
Author

Thank you very more for the detailed reply and all the information you provided. I did look at oneliner and it does more than what I had in mind right now for the Bioconductor-friendly style built on top of the tidyverse-style.

In the end, I made a new R package called biocthis at https://lcolladotor.github.io/biocthis/ and a bioc_style() function https://lcolladotor.github.io/biocthis/reference/bioc_style.html based on the code I mentioned previously. That is enough for my purposes right now as I can use it as shown at https://github.com/lcolladotor/biocthis/blob/fe66af4a12334559fc7d7968e53de0e676cb8689/dev/04_update.R#L9-L22

Again, thank you for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants