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

Keep at most one empty line at the end of each example #880

Merged
merged 3 commits into from
Dec 24, 2021

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Dec 22, 2021

This is crucial for combining multiple @examples and @examplesIf elements.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 22, 2021

I am not sure I can follow. Can you provide a new test to demonstrate the expected behavior? I.e. in and out files?

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 5e82878 is merged into main:

  • ❗🐌cache_applying: 28.3ms -> 32.6ms [+0.96%, +29.85%]
  •   :ballot_box_with_check:cache_recording: 992ms -> 1.02s [-1.04%, +6.74%]
  •   :ballot_box_with_check:without_cache: 2.65s -> 2.67s [-2.06%, +3.58%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

Also, we don't squash examples anymore: #747

@krlmlr
Copy link
Member Author

krlmlr commented Dec 22, 2021

Thanks. The "examples" are tibble and pillar -- try styling e.g. add.R in tibble with this PR and with the main branch. I'll add a formal test tomorrow.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 22, 2021

Ok, I see. But what's the rationale of having a blank line like between roxygen comments this? From tibble, (add.R).

#' Add columns to a data frame
#'
#' This is a convenient way to add one or more columns to an existing data
#' frame.
#'
#' @param .data Data frame to append to.
#' @param ... <[`dynamic-dots`][rlang::dyn-dots]>
#'   Name-value pairs, passed on to [tibble()]. All values must have
#'   the same size of `.data` or size 1.
#' @param .before,.after One-based column index or column name where to add the
#'   new columns, default: after last column.
#' @inheritParams tibble
#' @family addition
#' @examples
#' # add_column ---------------------------------
#' df <- tibble(x = 1:3, y = 3:1)
#'
#' df %>% add_column(z = -1:1, w = 0)
#' df %>% add_column(z = -1:1, .before = "y")
#'
#' # You can't overwrite existing columns
#' try(df %>% add_column(x = 4:6))
#'

#' # You can't create new observations
#' try(df %>% add_column(z = 1:5))
#'
#' @export
add_column

@krlmlr
Copy link
Member Author

krlmlr commented Dec 23, 2021

No, the blank line is bogus and can be removed. This is about the empty line before #' @export, the help page contains other functions with more examples, and I need to keep that empty line to visually separate the examples from each other.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8d11f22 is merged into main:

  •   :ballot_box_with_check:cache_applying: 30.9ms -> 31.1ms [-0.63%, +1.95%]
  •   :ballot_box_with_check:cache_recording: 1.09s -> 1.1s [-0.22%, +1.01%]
  •   :ballot_box_with_check:without_cache: 2.91s -> 2.92s [-0.36%, +1.13%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
Copy link
Member Author

krlmlr commented Dec 23, 2021

I have updated the test output files, there's still trouble. Could you please take a look?

Also, there are "unknown or unitialised token column" warnings, I have seen them before with CRAN styler.

I do wonder why this change adds so many empty lines for some outputs, in particular in the presence of \dontrun{}.

@krlmlr
Copy link
Member Author

krlmlr commented Dec 23, 2021

Empty lines look good to me now. Would you support this change?

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 83983f8 is merged into main:

  •   :ballot_box_with_check:cache_applying: 28.6ms -> 28.8ms [-0.67%, +1.62%]
  •   :ballot_box_with_check:cache_recording: 1.01s -> 1.01s [-0.82%, +0.61%]
  •   :ballot_box_with_check:without_cache: 2.69s -> 2.69s [-0.38%, +0.66%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@krlmlr
Copy link
Member Author

krlmlr commented Dec 23, 2021

Regarding the tidyverse style guide: not sure, it just feels that examples would be too condensed without empty lines inbetween.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 23, 2021

If I understand you correctly from your last three comments, all we need is to keep an empty line at the end of an example section if already one or more empty lines are present (empty as in only #' on that line, not completely empty). If there is none, we should not add one. Correct? Next time, I'd appreciate an issue and reprex to make things clearer in the now and if we revisit the issue later. 😄

@krlmlr
Copy link
Member Author

krlmlr commented Dec 23, 2021 via email

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 4f73008 is merged into main:

  •   :ballot_box_with_check:cache_applying: 36.6ms -> 36.9ms [-2.79%, +4.23%]
  •   :ballot_box_with_check:cache_recording: 1.47s -> 1.48s [-1.63%, +2.26%]
  •   :ballot_box_with_check:without_cache: 3.99s -> 4.01s [-0.95%, +2%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

The warning about unknown or uninitialized column seems to happen in various places in the code (as you can tell from the warnings). And it's not new. Probably it's similar to this:

x <- tibble::tibble()
x$ff[logical()] <- 1
#> Warning: Unknown or uninitialised column: `ff`.

Created on 2021-12-23 by the reprex package (v2.0.1)

For some reason, pd has sometimes now rows and the condition for the assignment is logical(), which has length 0.

Also, don't modify the .*-utf8 reference objects. I don't know why they don't fail on CRAN, but they fail for me locally too. Wird uns wohl irgendwann um die Ohren fliegen 😄

@lorenzwalthert
Copy link
Collaborator

@krlmlr can you have a look at the new test case I added and if that's what you want, you can merge it 🥳

Copy link
Member Author

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Great, thanks!

NEWS.md Outdated Show resolved Hide resolved
@krlmlr
Copy link
Member Author

krlmlr commented Dec 24, 2021

Are you suggesting that pd has no columns sometimes? Even if it has no rows and the column exists, the assignment works:

x <- tibble::tibble(ff = numeric())
x$ff[logical()] <- 1
x
#> # A tibble: 0 × 1
#> # … with 1 variable: ff <dbl>

Created on 2021-12-24 by the reprex package (v2.0.1)

We should normalize the data early in the process so that the structure is stable.

Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
@lorenzwalthert lorenzwalthert merged commit be6449c into main Dec 24, 2021
@lorenzwalthert lorenzwalthert deleted the f-empty-example-lines branch December 24, 2021 08:01
@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 448a7a8 is merged into main:

  •   :ballot_box_with_check:cache_applying: 29.4ms -> 29.6ms [-2.04%, +3.24%]
  •   :ballot_box_with_check:cache_recording: 1.03s -> 1.04s [-2.71%, +4.59%]
  •   :ballot_box_with_check:without_cache: 2.75s -> 2.77s [-2.92%, +4.68%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

Are you suggesting that pd has no columns sometimes?

Well, it has columns. But I think the problem is that it does not habe the ones to which something is assigned. Comparing my example with yours, yours already has the column that is assigned to. I’ll open a nee issue.

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.

2 participants