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

Bad error handling for invalid Lint()s #763

Closed
AshesITR opened this issue Feb 17, 2021 · 2 comments · Fixed by #1788
Closed

Bad error handling for invalid Lint()s #763

AshesITR opened this issue Feb 17, 2021 · 2 comments · Fixed by #1788
Labels
feature a feature request or enhancement

Comments

@AshesITR
Copy link
Collaborator

When a linter returns a Lint with an invalid (NA) column_number, as has recently happened in some regex-based linters, the error message is extremely confusing:

Error in rep.int(character, length) : invalid 'times' value
Calls: <Anonymous> ... print.lint -> cat -> highlight_string -> fill_with -> paste0
Execution halted

IMO Lint() should do some mild verification during construction so that invalid Lint()s can't be created successfully.

Here are a bunch of checks we could add:

  1. No NAs are allowed anywhere.
  2. 1L <= column_number <= nchar(line) + 1L.
  3. line_number >= 1L.
  4. ranges is a list of length 2 integer(ish) vectors, or NULL.
  5. 1L <= ranges[[i]][1L] <= ranges[[i]][2L] <= nchar(line) + 1L for all i.

WDYT?

@AshesITR AshesITR added the feature a feature request or enhancement label Feb 17, 2021
@MichaelChirico
Copy link
Collaborator

Would NULL be allowed for all of these? I'm thinking of lints that aren't as specific as the line/column level (e.g. file-level lints or line-level lints), which might make sense to leave one/both of line_number / column_number blank.

For ranges, don't we have cases where ranges[2L] might apply to a different line?

@AshesITR
Copy link
Collaborator Author

IINM NULLs are allowed in some places, but ranges are constrained to a single line only (or NULL), as we can currently only flag one line.

Line and column numbers could default to 1 as that would always be valid.

@AshesITR AshesITR added this to the 3.0.1 milestone Jul 27, 2022
AshesITR added a commit that referenced this issue Dec 3, 2022
@AshesITR AshesITR linked a pull request Dec 3, 2022 that will close this issue
AshesITR added a commit that referenced this issue Dec 8, 2022
* safer Lint(), xml_nodes_to_lints() and lint()

* add missing newline

* add more checks to Lint()

fixes #763

* fix surfaced errors in our own code

* fix cyclocomp

* avoid apply(..., simplify = FALSE) (R >= 4.1), try fixing r-devel

* delint

* line should have been line_number

* more line fixups

* reorder column == 0 case

* fix column number problems in r-devel

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)

* feedback

* fix tests

* fix apparent typo

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
@MichaelChirico MichaelChirico modified the milestones: 3.0.3, 3.1.0 Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants