-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conversation
reflow_comment() removed. wrapping behavior removed from mask_comments() and moved to tidy_block()
R/tidy.R
Outdated
x = unlist(lapply( | ||
x, | ||
function(line){ | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
How tests fail currently: in
spaces are added between the
Seems to have been pasted with |
There was a problem hiding this 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
\n
s after1+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){ | ||
|
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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")', |
There was a problem hiding this comment.
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.
…by unmask_source() again later
…le to do wrap = FALSE)
…simplify the code
There was a problem hiding this 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!
Wraps comment lines after indentation is performed on a comment line to keep it confined to specified width.