Skip to content

Conversation

@AshesITR
Copy link
Collaborator

fixes #507

  • implement fix
  • add regression test
  • add NEWS bullet

@AshesITR AshesITR requested a review from russHyde February 15, 2021 19:02
@AshesITR
Copy link
Collaborator Author

Diff coverage needs a test for declared_globals to pass.

@infotroph
Copy link
Contributor

I've only tried this fix on one (very messy) file so far, but lintr is exiting with a new error before it gets to the multiline expression I wanted to test:

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

When linting with the released version of lintr the next message emitted is a "Lines should not be more than 80 characters" -- if rep.int doesn't immediately tip you off I'll try to narrow this down to a reprex tonight.

@AshesITR
Copy link
Collaborator Author

@infotroph

This seems vaguely related to #742.
Can you narrow down which linter produces NA column_numbers?
as.data.frame(lint(...)) should help you find the issue.

# Conflicts:
#	R/object_usage_linter.R
#	tests/testthat/test-object_usage_linter.R
@AshesITR
Copy link
Collaborator Author

We're still getting NA locations if the info doesn't match on the first line of the checkUsage info.
I'll work on making the regex more robust.

@infotroph
Copy link
Contributor

Now working without error and with correct messages for the case I reported above, but for completeness I can report narrowed the offending lines down to an if-else with unbound names in both conditions:

fn <- function() {
  if (badname) {
  } else if (badname2) {
  }
}

Again, as of 9209e7a lintr is correctly warning on both lines and not erroring.

@AshesITR
Copy link
Collaborator Author

Thanks for checking this out @infotroph.
@MichaelChirico PTAL, I think it's ready to merge now.

foo <- function(x) {
with(
x,
`\u2019regex_kill`
Copy link
Collaborator

Choose a reason for hiding this comment

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

the test here is a symbol name that's not matched by our regex? so that the linter has to default to where the expression starts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The test causes the boundary, res$name, boundary regex to not match against the source code, yes.

@AshesITR AshesITR merged commit b1d4d0d into master Feb 18, 2021
@AshesITR AshesITR deleted the fix/507-multiline-object-usage branch February 18, 2021 09:05
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.

Missing object_usage_linter due to line break

4 participants