Skip to content

Conversation

jplecavalier
Copy link
Contributor

Fixes #1957.

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

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

Looking good! I had a couple comments/questions.

R/helpers.R Outdated
Comment on lines 67 to 68
direction <- ifelse(version_spec(version) > version_spec(existing_version),
"Increasing", "Decreasing")
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to an if () ... else? I generally reserve ifelse() for vectorized situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

R/helpers.R Outdated
"Increasing", "Decreasing")

ui_bullets(c(
"v" = "{.drn {direction}} {.pkg {package}} version to {.val {version}} in
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with the .drn inline markup. Can you help me understand that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried to replicate the same logic as in {.pkg {package}} and{.val {version}}. I tried to remove .drn and then {direction} would not resolve with the string in the object direction. Any hint?

Copy link
Member

Choose a reason for hiding this comment

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

Something like {.pkg {package}} does interpolation of package through its surrounding brackets. Then .pkg and the second layer of brackets applies some inline styling.

For pure interpolation (which is the goal with direction), we just need one pair of brackets.

You can learn more about this in the cli package: https://cli.r-lib.org/reference/inline-markup.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, and thanks again for your time and patience today!

@jennybc jennybc merged commit a225a21 into r-lib:main Aug 15, 2024
12 checks passed
@jplecavalier jplecavalier deleted the decrement-min-version branch August 16, 2024 02:52
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.

use_package() min_version will not decrement
2 participants