converted large_file_skip_byte_limit value to integer before comparison, fixes Issue #6847 #6848
Conversation
There was a problem hiding this comment.
You're on the right lines. Can I ask you to do a couple of extra things:
- Make a test case which covers the change you're making. I can give you some pointers if you're stuck, but if you have a look through the current test suite you'll probably be able to work out something. It should ideally cover setting this value to
None,100,0and"this_is_not_an_int". - Add a better error message here if the conversion fails, so users know what to do about it if it happens?
src/sqlfluff/core/linter/linter.py
Outdated
| limit = file_config.get("large_file_skip_byte_limit") | ||
| limit = int(file_config.get("large_file_skip_byte_limit")) |
There was a problem hiding this comment.
I like the approach, but at the moment this will probably just introduce another hard to ready error if someone puts in the wrong value for the config.
In particular, if the value isn't set then we'll get an error because int(None) is a ValueError think.
There was a problem hiding this comment.
That's a good point. Originally this option was caught by an if limit: statement right below. I have moved the conversion in there and also added some more descriptive error messages in there using a try statement
Coverage Results ✅ |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
|
Still got some linting errors |
|
@WittierDinosaur Thanks for rerunning. I think I've fixed the linting errors now so I hope it can pass now. Is there a way for me to test this myself btw? Then I wouldn't have to ask a contributor every time. |
|
Linting here is still an issue. If you have tox installed, you can run More info on setting things up can be found in the contributing docs. |
|
I once again believe I have fixed all linting issues with this PR, against all previous experience |
Head branch was pushed to by a user without write access
I didn't even know commas in these places wouldn't break the code
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
…on, fixes Issue sqlfluff#6847 (sqlfluff#6848) Co-authored-by: Danny Jones <51742311+WittierDinosaur@users.noreply.github.com> Co-authored-by: Alan Cruickshank <alanmcruickshank@gmail.com> Co-authored-by: Cameron <105471409+keraion@users.noreply.github.com>
In the linter, converted the value for large_file_skip_byte_limit to an integer to prevent the error in the comparison
This should fix Issue #6847 with no side effects.
Pull Request checklist
Please confirm you have completed any of the necessary steps below.
Included test cases to demonstrate any code changes, which may be one or more of the following:
.ymlrule test cases intest/fixtures/rules/std_rule_cases..sql/.ymlparser test cases intest/fixtures/dialects(note YML files can be auto generated withtox -e generate-fixture-yml).test/fixtures/linter/autofix.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.