Revise warnings with fixing unfixable files. #6257
Conversation
Coverage Results ✅ |
| @@ -118,16 +118,39 @@ def apply_fixes( | |||
| dialect: "Dialect", | |||
| rule_code: str, | |||
| fixes: Dict[int, AnchorEditInfo], | |||
| fix_even_unparsable: bool = False, | |||
There was a problem hiding this comment.
One thing that confuses me -- if a file has parsing errors and fix_even_unparsable is false, should this function even be called? There may be some nuance here I don't understand or have forgotten, but my initial instinct suggests there may be a more sweeping upstream fix possible. (By "sweeping", I mean at the file level rather than the parse subtree level.)
There was a problem hiding this comment.
Good point 🤔 .
When I check now, what you're suggesting does actually already exist, but further up in the linting process. In lint_paths() we only persist the file if it's free of templating errors, which means you're kind of right.
Is that what you mean?
There was a problem hiding this comment.
@barrywhart - I think you're right that we need to rationalise how fix_even_unparsable is used across the codebase. On inspection it's pretty inconsistent, including how it interacts with fix and when it's read from config and when it's read from a keyword argument.
I think it's a bigger issue to fix, and I'm going to suggest doing that as a separate piece of work. I've make an issue to track it: #6264
This is a test to see whether this approach passes other tests for resolving the conversation on slack with warnings about fixes being attempted in an unparsable file. The original poster was seeing lots of the error message
Fixes for ... not applied, it would re-cause the same errorandFixes for ... not applies, as it would result in an unparsable file. Please report this as a bug with a minimal query which demonstrates this warning.The issue was that an unparsable file we being passed to
fix. Because it was already unparsable, the validation was always failing - and hence the error messages. However this isn't a bug with sqlfluff per-se, it's a bug with the sql file being passed in. The warning messages are still unhelpful however.We don't have many test cases around fixing unparsable files either, so this revises the warning logic a little, and also puts a test case around that functionality. I also managed to cover some code which was previous uncovered (whoop)!
The test case isn't ideal because I couldn't show different behaviour for using
fix_even_unparsable, but it's better that in was before, and hopefully we'll find a better test case in future.