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

Fix an offense creation for Lint/EmptyFile #8676

Merged
merged 1 commit into from Sep 14, 2020

Conversation

fatkodima
Copy link
Contributor

Closes #8669

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 11, 2020

Can you explain why those changes are needed? Personally, I don't quite get it.

@marcandre
Copy link
Contributor

I thought the HTML formatter was the culprit (or maybe even my rewriter code)

@fatkodima
Copy link
Contributor Author

The original code

range = source_range(processed_source.buffer, 1, 0)		
add_offense(range)

adds an offense to non existent (empty) source range - 1st line, 0-th column.
When generating html report (I didn't tested, but this can be a problem for other formatters), this https://github.com/rubocop-hq/rubocop/blob/9bc7908b1ad171b33ede24908acced724db6e8d4/lib/rubocop/formatter/html_formatter.rb#L118-L126

119 and 124 lines fails here

https://github.com/whitequark/parser/blob/c25585d8d91f3470d9cdfd6f3b6a553ad9a809fb/lib/parser/source/buffer.rb#L269-L271

because of fetch on empty source_lines.

add_global_offense adds a global offense, roughly speaking, not attached to any specific location and overrides source_line to empty string, so this eliminates the problem.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 11, 2020

Got it. Might be nice to integrate those global offenses with expect_offence, though, as I'm certain people will wonder why are the tests using it.

@fatkodima
Copy link
Contributor Author

Might be nice to integrate those global offenses with expect_offence

syntax_spec.rb uses the same style, as this test file, so also needs an update.
Wdyt about introducing it in a separate PR?

@marcandre
Copy link
Contributor

I think:

source_range(processed_source.buffer, 1, 0)
# should be
source_range(processed_source.buffer, 1, 0, 0)	

FWIW, this helper is terrible. There are too many of these multiple argument public methods... The cop in charge of that should really have different settings for private (I don't care much, really) and public (should be max 1 imo).

If you prefer to go the add_global_offense route (seems a bit more complicated), I think the odd spec is fine. add_global_offense was really meant for non-parseable files tbh.

@fatkodima
Copy link
Contributor Author

fatkodima commented Sep 11, 2020

source_range(processed_source.buffer, 1, 0, 0)

Just tested, the same error as in the original issue.

@bbatsov bbatsov merged commit 4b6b6c8 into rubocop:master Sep 14, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Sep 14, 2020

Well, I guess the suggested fix will do for now. Thanks!

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.

Lint/EmptyFile throws error when html formatter is used
3 participants