Line-based ignores #8
Conversation
Structure: {"<file-name>" : [<line-number>]}
Will normally be set through the per-file configuration system, but
still has to be part of the global configuration, as said system applies
an overlay on top of the global config.
'_issue_to_file_location' is now called '_resolve_location', and consumes an absolute position instead of an issue. This is essential when determining the line number for syntax errors.
Used to determine if an issue should be reported.
|
Nice! Honestly this is a far more minimal change than I was expecting this to take. I'll give this a proper review later, but after a quick reading I didn't notice any changes that I'd be uncomfortable with. |
|
Have you been able to do a deep dive yet? 🙂 |
There was a problem hiding this comment.
Ah! Sorry for the delay, I was assuming you would be making more changes before wanting a review. Misunderstood 😄
Left a couple of minor comments, but overall looks good! Definitely want to make sure we're covered by tests before this gets landed.
To me this feels like the right place to make the change. Makes sense that the linting part of the code handles what gets raised as an issue, and the reporting part stays dumb and just displays issues in whichever format
| _location_for_issue(issue), | ||
| self._sql | ||
| ) | ||
| i = issue._replace(file=self._file_name)._replace( |
There was a problem hiding this comment.
nit: instead of chaining, let's merge the two calls to _replace
| @@ -149,17 +93,13 @@ def _format_message(issue): | |||
| return issue.message.format() | |||
|
|
|||
|
|
|||
| def _issue_info(issue, file_contents): | |||
| def _issue_info(issue): | |||
There was a problem hiding this comment.
I like this change, it makes sense to me that the Issue knows where it was raised from, rather than the reporting framework trying to figure it out after the fact
| line = contents[line_start:line_end].replace("\r", "") | ||
| column = location - line_start | ||
|
|
||
| return (line, line_num, column) |
| # Handle 'skip_file' after 'skip_lines', as it should be seen as | ||
| # an override | ||
| if rules['skip_file']: | ||
| skips = {**skips, file_name: []} |
There was a problem hiding this comment.
Feels a little off here to overload skips to hold meaning for both skipping lines and skipping the entire file.
| if file_config is None: | ||
| file_config = config.apply_file_config(base_config, file_name, contents) | ||
| # The file should be skipped if no specific line is ignored | ||
| if file_config.skip.get(file_name) == []: |
There was a problem hiding this comment.
So if I'm reading this right, line-based skips would take precedence over skipping the entire file? As a user, I think my expectation would be the opposite
| # Disregard specific rule for now | ||
| # Starts with 0, +1 additional line because | ||
| # we want to skip the *next* line | ||
| rules['skip_lines'].append(line_num + 2) |
There was a problem hiding this comment.
This is definitely bikeshedding, and I know you're just extending what I had for disabling files, but maybe this should be called disabled_lines to align with the flag that's used in the comment?
Adds line-based ignores as discussed in #7.
I didn't bother to write tests for this yet, as I feel that I made some rather substantial changes to the codebase that you might not agree with 😅
Adds the
skipattribute to the global config that can either be set through the global configuration, or by the per-file configuration system (which is much more likely).