safer Lint(), xml_nodes_to_lints() and lint() #1788
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1788 +/- ##
==========================================
- Coverage 98.89% 98.84% -0.06%
==========================================
Files 111 111
Lines 4713 4756 +43
==========================================
+ Hits 4661 4701 +40
- Misses 52 55 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Related to #763 as well. |
|
Funny how many bugs this change uncovered in our own codebase 😅 |
move column_number to nchar(line) + 1L if it's larger anchor end-of-line parse error lints to nchar(line) + 1 as well instead of nchar(line)
AshesITR
force-pushed
the
fix/1427-errors
branch
from
f635696 to
7d1bdad
Compare
|
@IndrajeetPatil @MichaelChirico this is ready for review now, I think. |
|
The linked issues are milestone 3.0.3, so moving this PR to 3.0.3 as well. |
|
Thanks for this! Mostly nit-picky comments, sorry about that. 95% of the way there 🤞 |
I'm confused now. From #1476, I understood that there isn't going to be a |
|
Yes, but the 3.1.0 milestone contains a lot of issues tabled for later at the moment. |
| #' | ||
| #' @noRd | ||
| fixup_line <- function(line) { | ||
| nchars <- tryCatch(nchar(line, type = "chars"), error = function(e) NA_integer_) |
There was a problem hiding this comment.
tempting to use NA here, since we're only checking is.na(), since technically logical type could be smaller than integer... OTOH maybe NULL would allocate even less space. OTOOH this doesn't really matter. Moving on.
There was a problem hiding this comment.
pryr::object_size(NA)
#> 56 B
pryr::object_size(NA_integer_)
#> 56 BCreated on 2022-12-08 with reprex v2.0.2
I'd prefer type safety (nchars is always an integer) instead of code brevity here.
R/lint.R
Outdated
| @@ -382,7 +382,7 @@ reorder_lints <- function(lints) { | |||
| #' Create a `lint` object | |||
| #' @param filename path to the source file that was linted. | |||
| #' @param line_number line number where the lint occurred. | |||
| #' @param column_number column number where the lint occurred. | |||
| #' @param number column number where the lint occurred. | |||
There was a problem hiding this comment.
is that intentional? this would be a user-facing API change? I guess this is by mistake...
| @@ -418,6 +433,35 @@ Lint <- function(filename, line_number = 1L, column_number = 1L, # nolint: objec | |||
| obj | |||
| } | |||
|
|
|||
| is_number <- function(number, n = 1L) { | |||
| length(number) == n && is.numeric(number) && !anyNA(number) | |||
There was a problem hiding this comment.
TIL length() is ever-so-slightly faster & so is better to test first. nice!
fixes #1427, and ensures an easier debugging experience because
Lint()errors immediately onNAinranges.xml_nodes_to_lints()uses default values if the location XPaths fail to return a number.This could also be made an error. WDYT?
Note that the current fallback solution doesn't guarantee col1 <= column_number <= col2.
Do we want to enforce this as well?