vscode-sqlfluff icon indicating copy to clipboard operation
vscode-sqlfluff copied to clipboard

Add quick fix for dbt rule L016 "line is too long" - add `-- noqa: L016` at EOL

Open Gregory108 opened this issue 2 years ago • 2 comments

If formatter is DBT, then, sometimes code creates too long lines that cannot be split. But it violates rule L016. This is the use case for -- noqa: L016 => suggest it as a quick fix in cases when this rule is violated.

Maybe: adding this quick fix prompt to all DBT rule violations Cons:

  • end users may start to abuse this new freedom and noqa everything
  • not all DBT rules are fixable by noqa, e.g. L044 "Somewhere along the “path” to the source data, specify columns explicitly"

Gregory108 avatar Nov 03 '22 12:11 Gregory108

I think adding that quick fix option for all violations would be a good idea. Leave it up to the user if they want to noqa everything. I don't think it would be too difficult to add either.

@Gregory108 Do you know if there is a list of rules that are not able to be disabled with noqa? If not I will probably add the prompt to all rules, which might be the way to go anyways.

RobertOstermann avatar Nov 03 '22 20:11 RobertOstermann

I am not aware of such a list, nor or the flag associated with the rule that marks local (un)fix-ability. *Hypothetically, it should be possible to derive it from the way parser works with the rule, but that seems to be an overkill.

  • Multiline violations may be quick fixed with this suggested syntaxis.
  • If there are, say, two violations on the same line, both noqa'ed, then it would be nice to have not repeated -- noqa: L000 -- noqa: L111, but more concise spelling as in an example. Some rules I checked are definitely same-line noqa "ingnorable", e.g. 009 and 050.

The following list of suspects - they, IMO, might either need multiline noqa or evaluate not a line, but piece of of code in entirety. *May have missed some; Some included may be resolvable in one-line. Please, specify, if you need this information or need this be double-checked

I walked through the rules for sqlfluff version 1.4.1. Here are the suspects to be non-fixable with the same-line -- noqa::

  • 015: "DISTINCT used with parentheses." - might be that parenthesis are on different lines and one noqa not enough/confuses the parser
  • 017: "Function name not immediately followed by parenthesis." - same suspicion, parenthesis on different lines
  • 019: "Leading/Trailing comma enforcement." - as in antipattern example, comma on the next line is "double" violation - of missing trailing comma on the line above and redundant leading comma on the line
  • 030: "Inconsistent capitalisation of function names." - not sure how violations are "assigned": inconsistent capitalisation are evaluated in the entirety of spelling variants
  • 032: "Prefer specifying join keys instead of using USING." - USING (key1, key2, key3) may span several lines
  • 034: "Select wildcards then simple targets before calculations and aggregates." - SELECT has to be evaluated in its entirety
  • 035: "Do not specify else null in a case when statement (redundant)." - else null might be multiline
  • 037: "Ambiguous ordering directions for columns in order by clause." - might be multiline
  • 038: "Trailing commas within select clause." - one strange, but possible problem might be that the comma is on a separate line from the last variable
  • 040: "Inconsistent capitalisation of boolean/null literal." - all uses throughout the code are taken into account by the rule
  • 041: "SELECT modifiers (e.g. DISTINCT) must be on the same line as SELECT." - the violation is multiline by nature
  • 042: "Join/From clauses should not contain subqueries. Use CTEs instead." - usually, multiline violation
  • 043: "Join/From clauses should not contain subqueries. Use CTEs instead." - usually, multiline violation
  • 044: "Query produces an unknown number of result columns." / "Somewhere along the “path” to the source data, specify columns explicitly" - evaluates the code in its entirety
  • 054: "Inconsistent column references in GROUP BY/ORDER BY clauses." - multiline by nature
  • 058: "Nested CASE statement in ELSE clause could be flattened." - multiline by nature
  • 063: "Inconsistent capitalisation of datatypes." - especially with parameter extended_capitalisation_policy="consistent" evaluates the code in its entirety
  • 064: "Consistent usage of preferred quotes for quoted literals." - especially with parameter preferred_quoted_literal_style="consistent"

Gregory108 avatar Nov 04 '22 08:11 Gregory108

@Gregory108 Thanks for putting together that list. I have release v0.1.8 that adds the noqa and noqa: <rule> options.

Currently, I have those options for all the rules, but my plan is to add a configuration option where you can disable rules so it won't show up for some of them. Then I will add those rules as the default to that list.

I could also add the noqa: disable=<rule> option for all rules that way you still have a way to disable those rules within the code.

Let me know your thoughts on how it works and if I should continue with the plan I outlined or if you think there is a better way to handle this.

RobertOstermann avatar Nov 05 '22 02:11 RobertOstermann

Completed partial testing. Single and compound one-line noqas work fine so far. Regarding UX: image It seems better to sort suggested options by frequency of use, which, I expect, would be:

  1. Ignore rule X for this line
  2. Ignore all rules for this line
  3. Exclude rule from Workspace settings
  4. Exclude rule from Global settings (btw, for new users it may be confusing what they are configuring by this action) .. after all, if one excluded the rule from Global settings, they would never need to ignore the rule for any line. One- vs multi-time-use options

Thank you for adding this shortcut! I did not expect it would take so much code (as I see in the commits of the Release)

Gregory108 avatar Nov 07 '22 07:11 Gregory108

Yeah, some of the code was just refactoring. Also, I already knew what needed to be added so it was not too difficult.

In release v0.1.9 I have added some additional configuration settings and re-ordered the rules as you suggested.

  "sqlfluff.codeActions.excludeRules.global": true,
  "sqlfluff.codeActions.excludeRules.workspace": true,
  // "sqlfluff.codeActions.noqa": false (to disable the code actions altogether)
  "sqlfluff.codeActions.noqa": [
    "L015","L017","L019","L030","L032","L034","L035","L037", "L038",
    "L040","L041","L042","L043","L044","L054","L058","L063","L064"
  ],

The default for sqlfluff.codeActions.noqa is the rules you listed so those rules will not see the noqa code actions. Probably needs a better setting name, but the description makes that more clear. Let me know if this is helpful.

I did a lot of refactoring in the v0.1.9 : Re-order code actions. Refactor code commit so if you are wanting to look at the changes the v0.1.9 : Implement sqlfluff.codeActions options to disable code actions has the addition of those settings and doesn't have refactoring.

RobertOstermann avatar Nov 08 '22 16:11 RobertOstermann