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

Convert roxygen2 comments to markdown #115

Merged
merged 14 commits into from
Nov 22, 2021
Merged

Convert roxygen2 comments to markdown #115

merged 14 commits into from
Nov 22, 2021

Conversation

Bisaloo
Copy link
Member

@Bisaloo Bisaloo commented Jul 23, 2021

No description provided.

@Bisaloo Bisaloo mentioned this pull request Jul 27, 2021
53 tasks
@Bisaloo Bisaloo marked this pull request as ready for review July 27, 2021 14:56
@Bisaloo Bisaloo force-pushed the review-roxygen2-md branch from fa012d5 to dcefbaa Compare July 27, 2021 15:05
@codecov
Copy link

codecov bot commented Jul 27, 2021

Codecov Report

Merging #115 (dcb3f27) into master (84361d9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #115   +/-   ##
=======================================
  Coverage   48.44%   48.44%           
=======================================
  Files          18       18           
  Lines        1348     1348           
=======================================
  Hits          653      653           
  Misses        695      695           
Impacted Files Coverage Δ
R/absolute_error.R 86.66% <ø> (ø)
R/bias.R 87.93% <ø> (ø)
R/eval_forecasts.R 66.07% <ø> (ø)
R/eval_forecasts_binary.R 100.00% <ø> (ø)
R/eval_forecasts_continuous_integer.R 69.64% <ø> (ø)
R/eval_forecasts_helper.R 100.00% <ø> (ø)
R/interval_score.R 95.45% <ø> (ø)
R/pairwise-comparisons.R 42.36% <ø> (ø)
R/pit.R 56.09% <ø> (ø)
R/plot.R 0.00% <ø> (ø)
... and 3 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 84361d9...dcb3f27. Read the comment docs.

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

nice stuff. A few comments. I see some issues in the check which will look at shortly.

R/bias.R Show resolved Hide resolved
R/bias.R Show resolved Hide resolved
#' the value in the \code{prediction} column, the value of \code{range}
#' should be 50 and the value of \code{boundary} should be "lower".
#' If you want to score the median (i.e. \code{range = 0}), you still
#' - `range` the range for which a forecast was made. For a 50%% interval
Copy link
Contributor

Choose a reason for hiding this comment

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

is %% correct markdown here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I seem to remember that %% is the correct way to escape % in LaTeX rather than \% but I cannot find any source for this so I might have invented it 🤷 😅.

Anyways, it seems that % doesn't need to be escape in markdown and that both \% and %% produce extra characters:

testfile <- tempfile(fileext = ".md")

write(
r"(
10%
20\%
30%%
)",
testfile
)

knitr::knit(testfile, output = stdout())
#> processing file: /tmp/RtmpPqcVnl/file301a18fe1273.md
#> 
#> 10%
#> 20\%
#> 30%%
#> A connection with                            
#> description "output"        
#> class       "textConnection"
#> mode        "wr"            
#> text        "text"          
#> opened      "opened"        
#> can read    "no"            
#> can write   "yes"

Created on 2021-08-04 by the reprex package (v2.0.0.9000)

Thanks for making me check!

R/eval_forecasts_continuous_integer.R Show resolved Hide resolved
R/eval_forecasts_continuous_integer.R Outdated Show resolved Hide resolved
R/interval_score.R Outdated Show resolved Hide resolved
R/pairwise-comparisons.R Show resolved Hide resolved
@seabbs
Copy link
Contributor

seabbs commented Aug 4, 2021

I've never read this and perhaps explains why my documentation is shocking: https://cran.r-project.org/web/packages/roxygen2/vignettes/rd-formatting.html

Key bits of the failing check:

Warning:   prepare_Rd: man/range_example_data_wide.Rd:19: unknown macro '\item'
196
  prepare_Rd: man/range_example_data_wide.Rd:20: unknown macro '\item'
197
214
  prepare_Rd: man/range_example_data_wide.Rd:40: unexpected section header '\usage'
215
  prepare_Rd: man/range_example_data_wide.Rd:43: unexpected section header '\description'
216
  prepare_Rd: man/range_example_data_wide.Rd:47: unexpected section header '\keyword'
217
  prepare_Rd: man/range_example_data_wide.Rd:48: unexpected END_OF_INPUT '
218
  '
219
  checkRd: (5) range_example_data_wide.Rd:0-68: Must have a \description
220

Warning:   Package unavailable to check Rd xrefs: ‘surveillance’
225
  Missing link or links in documentation object 'ae_median_quantile.Rd':
226
    ‘absolute_error’
227
230
❯ checking for code/documentation mismatches ... WARNING
231
Warning:   Data codoc mismatches from documentation object 'range_example_data_wide':
232
  Variables in data frame 'range_example_data_wide'
233
    Code: creation_date geography horizon lower_0 lower_10 lower_20
234
          lower_30 lower_40 lower_50 lower_60 lower_70 lower_80 lower_90
235
          model true_value upper_0 upper_10 upper_20 upper_30 upper_40
236
          upper_50 upper_60 upper_70 upper_80 upper_90 value_date
237
          value_desc value_type
238
    Docs: creation_date geography horizon lower_0 model true_value
239
          value_date value_desc value_type
240

Mainly moving from \ to @ I think

R/scoringutils.R Outdated Show resolved Hide resolved
@nikosbosse
Copy link
Contributor

nikosbosse commented Aug 4, 2021 via email

@Bisaloo
Copy link
Member Author

Bisaloo commented Aug 4, 2021

The solution for failing tests would be to either:

  • removing the link to surveillance::permutationTest()
  • add the permutation package to Suggests

Both solutions seem as good from a technical point of view. I'd say it depends if you want to advertise the surveillance package or not. So I leave the choice to you.

@nikosbosse nikosbosse merged commit 29ac025 into master Nov 22, 2021
@nikosbosse nikosbosse deleted the review-roxygen2-md branch November 22, 2021 09:04
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.

3 participants