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

Correctly Wrap Comments According to Width Cutoff #92

Merged
merged 14 commits into from May 25, 2021
Merged

Conversation

iqis
Copy link
Contributor

@iqis iqis commented Apr 16, 2021

Wraps comment lines after indentation is performed on a comment line to keep it confined to specified width.

Siqi Zhang added 2 commits April 15, 2021 21:12
reflow_comment() removed.
wrapping behavior removed from mask_comments() and moved to tidy_block()
R/tidy.R Outdated
x = unlist(lapply(
x,
function(line){

Copy link
Contributor Author

Choose a reason for hiding this comment

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

too sparse here? remove empty lines?

Copy link
Owner

Choose a reason for hiding this comment

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

You can leave the cosmetic stuff aside for now.

R/utils.R Outdated
@@ -41,27 +41,9 @@ mask_comments = function(x, width, keep.blank.line, wrap = TRUE, spaces) {
c0 = d.line[-1] != d.line[-n] # is there a line change?
c1 = i & c(TRUE, c0 | (d.token[-n] == "'{'")) # must be comment blocks
c2 = i & !c1 # inline comments
Copy link
Contributor Author

Choose a reason for hiding this comment

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

c3, an index used to distinguish roxygen comments, is no longer necessary.

@@ -18,7 +18,7 @@ replace_assignment = function(exp) {
}

## mask comments to cheat R
mask_comments = function(x, width, keep.blank.line, wrap = TRUE, spaces) {
mask_comments = function(x, keep.blank.line, spaces) {
d = utils::getParseData(parse_source(x))
if (nrow(d) == 0 || (n <- sum(d$terminal)) == 0) return(x)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This n <- sum(d$terminal part in side the condition is stinky in more than one ways. Change?

Copy link
Owner

Choose a reason for hiding this comment

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

I often use this style myself (assignment and condition in one expression). Do you mean this?

Suggested change
if (nrow(d) == 0 || (n <- sum(d$terminal)) == 0) return(x)
if (nrow(d) == 0) return(x)
n = sum(d$terminal)
if (n == 0) return(x)

Usually I prefer relatively dense code, so I don't need to scroll up and down frequently.

R/tidy.R Outdated
line = sub(indent_and_comment_characters, '', line)

# wrap line
line = unlist(strwrap(line,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe it should be simplify = TRUE with no unlist() but I was afraid I might miss something here.

Copy link
Owner

Choose a reason for hiding this comment

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

Try to go without unlist() + simplify = FALSE. I guess it should be fine.

@iqis
Copy link
Contributor Author

iqis commented Apr 16, 2021

How tests fail currently:

in test-tidy.R

> tidy.res(c('if(TRUE){1+1', '', '}', '', '# a comment'))
[1] "if (TRUE) {\n    1 + 1\n     \n}" " "                               
[3] "# a comment"    

spaces are added between the \ns after 1+1 compared to
c('if (TRUE) {\n 1 + 1\n\n}', '', '# a comment')

> tidy.res(x1, width.cutoff = 20)
[1] "# a b c d e f g h i\n# j k l m n o p q r\n# s t u v w x y z"

Seems to have been pasted with \n while the desired is
c('# a b c d e f g h i j', '# k l m n o p q r s t', '# u v w x y z')

Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

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

How tests fail currently:

in test-tidy.R

> tidy.res(c('if(TRUE){1+1', '', '}', '', '# a comment'))
[1] "if (TRUE) {\n    1 + 1\n     \n}" " "                               
[3] "# a comment"    

spaces are added between the \ns after 1+1 compared to
c('if (TRUE) {\n 1 + 1\n\n}', '', '# a comment')

Sound like you need to trimws() in tidy_block().

> tidy.res(x1, width.cutoff = 20)
[1] "# a b c d e f g h i\n# j k l m n o p q r\n# s t u v w x y z"

Seems to have been pasted with \n while the desired is

c('# a b c d e f g h i j', '# k l m n o p q r s t', '# u v w x y z')

Let's update the tests. Thanks!

R/tidy.R Outdated
x = unlist(lapply(
x,
function(line){

Copy link
Owner

Choose a reason for hiding this comment

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

You can leave the cosmetic stuff aside for now.

R/tidy.R Outdated
unlist(lapply(exprs, function(e) {
x = deparse(e, width)
x = reindent_lines(x, indent)
x = unlist(lapply(
Copy link
Owner

Choose a reason for hiding this comment

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

I'd move this part into a separate function like the previous reflow_comments().

R/tidy.R Outdated
line = sub(indent_and_comment_characters, '', line)

# wrap line
line = unlist(strwrap(line,
Copy link
Owner

Choose a reason for hiding this comment

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

Try to go without unlist() + simplify = FALSE. I guess it should be fine.

R/tidy.R Outdated
# re-mask line
line = unlist(lapply(line,
function(line){
sprintf('invisible("%s%s%s")',
Copy link
Owner

Choose a reason for hiding this comment

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

Do we still need to put the comments back into masks? I feel that's not necessary.

@yihui
Copy link
Owner

yihui commented May 24, 2021

@iqis Just to let you know, I plan to make a new CRAN release to support the native pipe (3f7656f) this week, so I'll see if I can finish revising this PR by myself and include it in the next release. Thanks!

@yihui yihui marked this pull request as ready for review May 25, 2021 03:00
Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Should be good to go now. Thanks!

@yihui yihui merged commit eb5cbf3 into yihui:master May 25, 2021
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.

None yet

2 participants