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

housekeeping #159

Merged
merged 10 commits into from
Sep 13, 2021
Merged

housekeeping #159

merged 10 commits into from
Sep 13, 2021

Conversation

IndrajeetPatil
Copy link
Member

@IndrajeetPatil IndrajeetPatil commented Sep 9, 2021

  • spell check
  • consistently use markdown syntax in roxygen docs
  • style code
  • vignette chunk label fixes
  • minor rephrasing of vignettes
  • update roxygen version
  • remove unnecessary pander from Suggests
  • follow best practices for .Rproject
  • get rid of warnings introduced in ggplot2 3.3.3

- spell check
- consistently use markdown syntax in roxygen docs
- style
- vignette chunk label fixes
- minor rephrasing
- update roxygen version
- remove unnecessary pander from suggests
- follow best practices for Rproject
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #159 (64376ca) into develop (f6044eb) will increase coverage by 0.00%.
The diff coverage is 23.52%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #159   +/-   ##
========================================
  Coverage    48.57%   48.57%           
========================================
  Files           55       55           
  Lines         2530     2532    +2     
========================================
+ Hits          1229     1230    +1     
- Misses        1301     1302    +1     
Impacted Files Coverage Δ
R/aggregation-input.R 90.69% <ø> (ø)
R/aggregation-summary.R 0.00% <0.00%> (ø)
R/atom-plots.R 80.42% <ø> (ø)
R/boxwhisker-datamapping.R 0.00% <ø> (ø)
R/boxwhisker-plotconfiguration.R 0.00% <ø> (ø)
R/datamapping-grouping.R 84.61% <ø> (ø)
R/datamapping-groupmapping.R 100.00% <ø> (ø)
R/datamapping-range.R 62.06% <ø> (ø)
R/datamapping-xy.R 30.76% <ø> (ø)
R/datamapping-xygroup.R 94.59% <ø> (ø)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6044eb...64376ca. Read the comment docs.

Copy link
Member

@msevestre msevestre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the changes are more than MD or style... Can you explain what it does?

tlf.Rproj Show resolved Hide resolved
@@ -1,7 +1,11 @@
---
title: "Atom plots"
author: "OSPSuiteR 2019"
output: rmarkdown::html_vignette
output:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IndrajeetPatil Why those changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • fig.align = 'center' Figures aligned to center look better.
  • dpi = 300 for better resolution of plots (markdown defaults to dpi = 72, which makes for a poor figure).
  • toc = TRUE to have a table of content, which makes it easier to navigate between different sections, which is especially useful if the vignette is long.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pchelle FYI: Probably something to use for ReportingEngine as well

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed

@IndrajeetPatil
Copy link
Member Author

@msevestre This is a draft PR, btw. When the PR is ready, I will ask for a review. :)

@msevestre
Copy link
Member

@msevestre This is a draft PR, btw. When the PR is ready, I will ask for a review. :)

Ok my bad :)

Copy link
Member

@Yuri05 Yuri05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert ggplot2 version to >=3.3.0
3.3.5 is not supported yet on some systems where tlf must run

DESCRIPTION Outdated Show resolved Hide resolved
@IndrajeetPatil
Copy link
Member Author

@Yuri05 Sure, will do that.

Out of curiosity, what are the systems where this ggplot2 version is not available?
At least on CRAN, I see that the binaries are built for macOS and Windows on oldrel, release, and devel.

image

@Yuri05
Copy link
Member

Yuri05 commented Sep 12, 2021

what are the systems where this ggplot2 version is not available?

ValidR by Mango. R itself and different packages are usually available in some older version.

@IndrajeetPatil IndrajeetPatil marked this pull request as ready for review September 13, 2021 09:24
@IndrajeetPatil
Copy link
Member Author

Now the PR is ready for a review 😅

@PavelBal
Copy link
Member

@Yuri05 @msevestre Don't wait for my review to merge, I have no idea about TLF :D

@Yuri05 Yuri05 requested review from pchelle and abdullahhamadeh and removed request for PavelBal September 13, 2021 13:14
Copy link
Collaborator

@pchelle pchelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, do you use any specific package for spell check of documentation ?
Also, what is the good practice between using \code{} and the back quotes ? (I feel I did not use those properly)

@IndrajeetPatil
Copy link
Member Author

Out of curiosity, do you use any specific package for spell check of documentation?

Yes, I use devtools::spell_check().

Also, what is the good practice between using \code{} and the back quotes? (I feel I did not use those properly)

Not best practice per se, but just a matter of syntactical taste:
If you want to use Rd syntax, it will be \code{}, while markdown syntax would be backticks.
Given the simplicity of markdown syntax, I am assuming that's what we prefer.

@msevestre
Copy link
Member

Given the simplicity of markdown syntax, I am assuming that's what we prefer.

yes. we just all assume we had to use the outdated syntax. Md for the WIN

@msevestre msevestre merged commit dbaa7a6 into Open-Systems-Pharmacology:develop Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants