Skip to content

Adding exception handling for invalid parse data. #139

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
merged 1 commit into from
Aug 22, 2017

Conversation

lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Aug 20, 2017

This closes #137.

For style_text()

library("styler")
style_text("a 3")
#> Error in parsing the supplied text. Here is the parsing error message: 
#> Error in parse(text = text, keep.source = TRUE): <text>:1:3: unexpected numeric constant
#> 1: a 3
#>       ^
```And for `style_file()` and friends.
```r
path <- tempfile(fileext = ".R")
writeLines("a 3", path)
style_file(path)
#> Error in parsing file /private/var/folders/8l/fhmv3yj12kncddcjqwc19tkr0000gr/T/RtmpSOgZBl/file728e3c9f60a9.R Here is the parsing error message: 
#> Error in parse(text = text, keep.source = TRUE): <text>:1:3: unexpected numeric constant
#> 1: a 3
#>       ^
#> --------------------------------------------------------------------------------

As you can see from the source code, it will still style all parsable files if there are any.

That's how it looks now. Much better I think.

@lorenzwalthert
Copy link
Collaborator Author

The only thing that bothers me a bit is that the return value is just a message, not an error, but I think there is no way around it if we want to continue with other files, right?

@lorenzwalthert lorenzwalthert force-pushed the exception_handling branch 2 times, most recently from ee0aacf to 02b8c79 Compare August 20, 2017 11:55
@lorenzwalthert lorenzwalthert changed the title Adding exeption handling for invalid parse data. Adding exception handling for invalid parse data. Aug 20, 2017
@codecov
Copy link

codecov bot commented Aug 20, 2017

Codecov Report

Merging #139 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
+ Coverage   87.46%   87.61%   +0.15%     
==========================================
  Files          19       19              
  Lines         742      751       +9     
==========================================
+ Hits          649      658       +9     
  Misses         93       93
Impacted Files Coverage Δ
R/ws.R 88.88% <100%> (+5.55%) ⬆️
R/transform.R 97.36% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9339ba...62fd6b7. Read the comment docs.

R/transform.R Outdated
#' @inheritParams make_transformer
#' @return A logical value that indicates whether or not any file was changed is
#' returned invisibly. If files were changed, the user is informed to
#' carefully inspect the changes via a message sent to the console.
transform_files <- function(files, transformers) {
transform_files <- function(files, transformers, assert_code = TRUE) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need a new argument here. Rather the behavior should always be reasonable if parsing fails. If necessary, we could change transform_lines_enc() to account for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so we can remove the argument then. I am not sure whether it makes sense to implement the error handling in transform_lines_enc() since it has already a error handling but I don't know why it does only give a warning instead of an error.

library("styler")
style_text("a 3")
#> Error in parse(text = text, keep.source = TRUE): <text>:1:3: unexpected numeric constant
#> 1: a 3
#>       ^
path <- tempfile(fileext = ".R")
writeLines("a 3", path)
style_file(path)
#> Warning: When processing file6b1f0f56be.R: <text>:1:3: unexpected numeric constant
#> 1: a 3
#>       ^
#> Error in if (any(changed)) {: missing value where TRUE/FALSE needed
style_dir(dirname(path))
#> Warning: When processing ./file6b1f0f56be.R: <text>:1:3: unexpected numeric constant
#> 1: a 3
#>       ^
#> Error in if (any(changed)) {: missing value where TRUE/FALSE needed

Copy link
Member

Choose a reason for hiding this comment

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

I forgot how transform_lines_enc() works, the error handling seems sufficient already. Do you need any(changed, na.rm = TRUE) ?

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, with any(changed, na.rm = TRUE), we only get the first warning message. So I will undo my error handling implementation and adapt the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my eyes, there is one more tweak necessary in the utf8 package. Unfortunately, it is not visible in the reprex above because reprex strangely does not show all warnings I get in the console. I opened #15 and submitted a PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, do you need to submit utf8 to CRAN in order to submit styler to CRAN, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm about to submit utf8.

@lorenzwalthert lorenzwalthert force-pushed the exception_handling branch 2 times, most recently from 86fd011 to b160f5a Compare August 21, 2017 17:16
and leave it to utf8::transform_lines_enc()
@lorenzwalthert
Copy link
Collaborator Author

Ok, I now reverted everything but kept the tests, so error handling is now done by utf8.

@lorenzwalthert lorenzwalthert requested a review from krlmlr August 22, 2017 11:10
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.

Awesome, thanks!

@lorenzwalthert lorenzwalthert merged commit f340918 into r-lib:master Aug 22, 2017
krlmlr added a commit that referenced this pull request Aug 24, 2017
- Vignette on customizing styler (#145).
- No line break after `switch()` and friends (#152).
- Remove flat relicts completely (#151).
- Don't reindent function calls and break line correctly for multi-line calls (#149).
- Set space between "=" and "," (#150).
- Make R CMD Check perfect (#148).
- Adding tests for exception handling with invalid parse data (#139).
- Fix indention by checking for all potential triggers (#142).
- Fix un-indention (#135).
- Support wide characters (#130).
- No spaces around :, :: and :::.
- Redesigning the API (#123).
- Solve eq_sub indention in general (#125).
- Minor refactorings.
- Re-indent token-dependent (#119).
- Supporting more indention patterns.
- Allow raw indention.
- Definitively fixing eol issue with comments.
- Infrastructure.
- Flattening out the parse table.
- New rule: no space after ! -> !!! for tidyeval.
- Fix spacing around '{'.
- Don't drop tokens! Fixes #101.
- EOL spaces in empty comments (and in general) (#98).
- mal-indention in conditional statement due to wrong specification of indent_without_paren) (#95).
- Complicated indentions based on arithmetic and special operators (#96).
- indention interaction on with assignment operator and other operators (#97).
@lorenzwalthert lorenzwalthert deleted the exception_handling branch September 17, 2017 20:55
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.

Exception Handling
2 participants