Skip to content

126 replace variable collapse with gluecollapse - #132

Merged
peterdesmet merged 5 commits into
mainfrom
126-replace-variable_collapse-with-gluecollapse
Mar 27, 2023
Merged

126 replace variable collapse with gluecollapse#132
peterdesmet merged 5 commits into
mainfrom
126-replace-variable_collapse-with-gluecollapse

Conversation

@PietrH

@PietrH PietrH commented Mar 22, 2023

Copy link
Copy Markdown
Member

Resolves #126

Moves some objects that were only used for messaging inside glue syntax.

@PietrH PietrH added the enhancement New feature or request label Mar 22, 2023
@PietrH PietrH added this to the 1.1.0 milestone Mar 22, 2023
@PietrH
PietrH requested a review from peterdesmet March 22, 2023 10:12
@PietrH PietrH self-assigned this Mar 22, 2023
@PietrH PietrH linked an issue Mar 22, 2023 that may be closed by this pull request
@PietrH

PietrH commented Mar 22, 2023

Copy link
Copy Markdown
Member Author

Ready for review

@peterdesmet peterdesmet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR moves the variables to the glue() function. It does not use glue_collapse() though. Do you see a benefit in using this over paste()?

@peterdesmet

Copy link
Copy Markdown
Member

E.g. could we use:

assertthat::assert_that(
    all(!is.na(field_names)),
    msg = glue::glue(
      "All fields in `schema` must have property `name`.",
      "\u2139 Field(s) `{field_numbers}` don't have a name.",
      .sep = "\n",
      field_numbers = glue::glue_collapse(which(is.na(field_names)), "`, `")
    )
  )

@PietrH

PietrH commented Mar 24, 2023

Copy link
Copy Markdown
Member Author

In this case I don't see a benefit, but not a real cost either. So we can certainly use glue_collapse() if you prefer.

@peterdesmet
peterdesmet merged commit f2045fa into main Mar 27, 2023
@peterdesmet
peterdesmet deleted the 126-replace-variable_collapse-with-gluecollapse branch June 20, 2023 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace variable_collapse with glue::collapse()

2 participants