L009: Handle adding newline after {% endif %} at end of file
#2862
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2862 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 163 163
Lines 12441 12458 +17
=========================================
+ Hits 12441 12458 +17
Continue to review full report at Codecov.
|
|
@alanmcruickshank: Tagging you for review if you have time, as this PR touches several core components I haven't worked on before. |
|
|
||
| def dedupe_tuple(self): | ||
| """Generate a tuple of this fix for deduping.""" | ||
| return (self.source_slice, self.fixed_raw) |
There was a problem hiding this comment.
Moved this class so BaseSegment can use it.
| # in which case, we should skip this patch. | ||
| continue | ||
| source_slice = getattr(patch, "source_slice", None) | ||
| if source_slice is None: |
There was a problem hiding this comment.
Compare this section with "Hide whitespace" enabled
|
|
||
| def dedupe_tuple(self): | ||
| """Generate a tuple of this fix for deduping.""" | ||
| return (self.source_slice, self.fixed_raw) |
There was a problem hiding this comment.
Moved this class here from src/sqlfluff/core/linter/common.py.
| @@ -289,7 +289,6 @@ def _slice_template(self) -> List[RawFileSlice]: | |||
| # parts of the tag at a time. | |||
| unique_alternate_id = None | |||
| alternate_code = None | |||
| trimmed_content = "" | |||
There was a problem hiding this comment.
Simple cleanup: This variable was unused.
There was a problem hiding this comment.
This is pretty good. The bulk of the structure makes total sense 👍 .
The bits that I think could be a little neater is the iterplay between segments.base.iter_patches() (which we've edited to also return EnrichedPatch) and linter.linted_file.LintedFile.fix_string() (which is the only function which calls it).
| # Generate placeholders for any source-only slices that *follow* | ||
| # the last element. This happens, for example, if a Jinja templated | ||
| # file ends with "{% endif %}", and there's no trailing newline. | ||
| if idx == len(elements) - 1: |
There was a problem hiding this comment.
This is neat. 👍
Very clean and well commented.
| # By returning an EnrichedFixPatch (rather than FixPatch), which | ||
| # includes a source_slice field, we ensure that fixes adjacent | ||
| # to source-only slices (e.g. {% endif %}) are placed | ||
| # appropriately relative to source-only slices.. | ||
| yield EnrichedFixPatch( |
There was a problem hiding this comment.
This feels a little weird, in that we're enriching some patches and not others - but it seems like a clean solution.
I've checked out the flow around it and I think it makes sense. I do wonder whether we should account more explicitly for potential EnrichedPatch objects being returned in fix_string() where I think in the current flow we might end up recreating them rather than just using the returned ones from iter_patches.
btw - I don't think it would be crazy to pass a TemplatedFile to iter_patches rather than just the templated_string. When it's called in fix_string it's given the latter from the former anyway. If we did that then you'd have access to the strings so you'd be able to populate the details below.
There was a problem hiding this comment.
I'll look into it. I had some of the same thoughts, but kind of suppressed them initially because I was focused on seeing if the whole thing was going to work. 😄
There was a problem hiding this comment.
@alanmcruickshank: Done! I also reworked things a bit to make EnrichedFixPatch a subclass of FixPatch. I think it's significantly cleaner now, and was a useful refactoring independent of the this PR.
tunetheweb
changed the title
{% endif %} at end of file
|
@alanmcruickshank, @tunetheweb: Ready for another review. No change in functionality, just some refactoring of the patch classes and handling in |
Brief summary of the change made
Fixes #2822
OMG, this is the deepest bug I've seen in a long time. Fixing it has required changes to:
JinjaTracerwith better handling of{% endif %}and{% endfor %}is_final_segment()to optionally consider meta segments and updating L009 to use itAre there any other side effects of this change that we should be aware of?
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.