Skip to content

avoid checking for hard-coded dot #262

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

Merged

Conversation

lorenzwalthert
Copy link
Collaborator

Can do without checking for "." as suggested in #81 by @krlmlr, but does not necessarily make things simpler, mainly because we used to check at a different nest, that is, we checked in the nest where token_before is "SPECIAL-PIPE", so we did not use the reduce approach since these nests only had one row. Closes #81.

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks. I think looking at the structure is more robust than looking at the text of an expression, this seems worth the extra complexity.

R/rules-other.R Outdated
pd <- bind_rows(pd, new_pd)
is_pipe <- pd$token == "SPECIAL-PIPE"
if (any(is_pipe, na.rm = TRUE)) {
pd <- reduce(which(is_pipe), add_brackets_in_pipe_one, .init = pd)
Copy link
Member

Choose a reason for hiding this comment

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

Would the reduce() call also work if which(is_pipe) has length zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thanks.

R/utils.R Outdated
#' @param pos The position of the token to start the search from.
#' @importFrom rlang seq2
next_non_comment <- function(pd, pos) {
if (pos >= nrow(pd)) return(integer(0))
Copy link
Member

Choose a reason for hiding this comment

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

This statement seems redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, in the current use context it seems, so I can remove it. Just wanted to add safety.

@lorenzwalthert lorenzwalthert merged commit dd73938 into r-lib:master Oct 26, 2017
@lorenzwalthert lorenzwalthert deleted the drop-hard-coded-text-check-for-pipe branch November 10, 2017 22:29
krlmlr added a commit that referenced this pull request Nov 27, 2017
- Adapt documentation (#290).
- Add roundtrip (#287).
- Fix AppVeyor builds.
- Fix token insertion / comment interaction (#279).
- Clarify labelling strategy (#285).
- Fixing and extending Rstudioaddins (#283).
- Fix eq assign parsing (#276).
- style_files -> vectorized style_file (#273).
- Refactoring (#270).
- Fix CI (#275).
- Fix covr (#274).
- Renaming files (#271).
- Handle styling of an unsaved active file (#243).
- Test R 3.1 and R 3.2 (#249).
- Allow empty {} without line break (#261).
- Wrap expr in expr before enclosing with curly braces (#263).
- Avoid checking for hard-coded dot (#262).
- Account for dependency renaming (utf8 changed to enc) (#264).
- Indention of function declaration and closing braces (#260).
- Only remove line break before closing with strict option (#252).
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.

2 participants