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

Improve the print method #296

Closed
wants to merge 6 commits into from
Closed

Improve the print method #296

wants to merge 6 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented Jan 27, 2023

Inspired by stringr::str_view()

Fixes #290

Inspired by stringr::str_view()

Fixes #290
R/glue.R Outdated Show resolved Hide resolved
R/glue.R Show resolved Hide resolved
R/glue.R Show resolved Hide resolved
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.

This feels like a pretty big change in the print method to me! So much so that it gives me pause.

I've always assumed that the main benefit of having the "glue" class was to get the quote-free, clutter-free printing and this walks away from that. I know we have some regrets about the "glue" class, but this seems to undermine its main rationale?

This almost feels more like a str() method. What do you think of me turning it into that?

(And, if so, yes I still think we need to do something when printing a glue object of length 0.)

@jennybc
Copy link
Member

jennybc commented Sep 24, 2023

I ran revdep checks and, as expected, only a saw a couple of failures due to this, including r-lib/testthat#1868. But in the course of figuring out that puzzle, I checked pillar locally and saw massive snapshot test change, which I think could be pretty widespread. I know that won't make tests fail on CRAN, in general, but it increases my hunch that we should introduce this display as a str() method.

@hadley
Copy link
Member Author

hadley commented Sep 25, 2023

We should maybe talk this through live as it's definitely not a str method. I think it stays true to the intent of glue to just "show the strings", while avoiding confusion in cases like this:

glue::glue("{x}", x = c("a", "b", "c\nd"))
#> a
#> b
#> c
#> d

Created on 2023-09-25 with reprex v2.0.2

@hadley
Copy link
Member Author

hadley commented Nov 22, 2023

I've resolved the remaining discussion items; @jennybc we should chat to confirm that this makes sense.

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.

I'm still not sure I'm on board with this, but we'll discuss in person. I'm comfortable with how it's done.


truncated <- length(x) > max
if (truncated) {
x <- x[seq_len(max)]
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to affect the return value? I.e. print() no longer passes x through invisibly.

@hadley
Copy link
Member Author

hadley commented Nov 29, 2023

See discussion in #290

@hadley hadley closed this Nov 29, 2023
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.

Consider an improved print method
3 participants