Revise as_cli to return a single string without \n characters#6866
Open
dmuenz wants to merge 3 commits into
Open
Revise as_cli to return a single string without \n characters#6866dmuenz wants to merge 3 commits into
dmuenz wants to merge 3 commits into
Conversation
…k characters * Technically if the ... text width is greater than 1e9 then we could get line breaks, but that's unrealistic. I considered setting cli.width to Inf, but the cli package documentation doesn't explicitly say that Inf is a supported value.
This test would have failed prior to as_cli being revised
The tests with "bad input" previously passed, but only because the default console width was wide enough to mask the problem with line breaks. By locally setting cli.width to a small number (20), the "bad input" tests would fail without the revision to as_cli.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attempts to fix #6789. This PR revises the internal helper function
as_cli, which is used to richly format text for printing to the console.Previously,
as_cli(...)could return a character vector of length > 1 if the text in...was longer than the console width. That's a problem for S7 property validators which expect a string (i.e. a length-1 character vector) for signaling an error. So this PR aims to fix that byusing the
cli::cli_fmt(collapse = TRUE, strip_newline = TRUE)options which ensure we get a string but which may have internal"\n"characters, andlocally setting the
cli.widthglobal option to an enormous number so thatcli::cli_fmtthinks the console is enormously wide and thus does not insert any"\n"s.The reason I don't want
cli::cli_fmtto insert newline characters is that the function generally doesn't have access to the complete text to be printed, and thus it will insert newlines in the wrong places. E.g., with S7 property validators, the text"- @prop_name "is prepended to the error message, but cli doesn't account for the length of that text which can lead to strange-looking line wrapping.Regarding testing, I added a new direct test of the revised
as_clibehaviour intest-utilities.R. I also revised a couple tests of S7 property validators intest-properties.Rwhich previously passed but only because they weren't testing the error messages with a narrow console width. Now the tests simulate a narrow console and they still pass but only because of the revision toas_cli.I ran the whole suite of package tests and they all passed (except for a few that were skipped because they're OS-specific), including tests of the behavior of other functions that use
as_cli, includingcheck_required_aestheticsandvalidate_subclass.I think this is ready for review at your convenience.