Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade SQLFluff to latest #3387

Closed
wants to merge 1 commit into from
Closed

Upgrade SQLFluff to latest #3387

wants to merge 1 commit into from

Conversation

tunetheweb
Copy link
Member

SQLFluff v2 contains a lot of breaking changes in the config, and also basically a complete rewrite of the formatting rules.

A previous attempt in #3364 couldn't be completed as we weren't happy with some of the changes necessary to support both SQLFluff v1 (as used by the Super Linter used by our GitHub Action ) and v2. Now the SQLFluff Linter has been upgraded we can do a (slightly!) cleaner and smaller change, though there are still a lot of changes.

The alternative is to stay on old, unsupported version but that's not a great idea for many reasons.

Comment on lines -25 to +26
id)
id
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunatelty v2 doesn't allow trailing closing brackets. But think that's OK as more clear now when line 8 closes.

Comment on lines -26 to 31
page),
page
),
UNNEST([10, 25, 50, 75, 90]) AS percentile
GROUP BY
percentile,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially a problem for us due to the "faux K&R" style we're following here.

Basically if you take this simplified version of this query:

SELECT
  *
FROM (
  SELECT * FROM a table
),
  UNNEST([...])

The FROM clause is made up of two tables: 1) a subquery and 2) an UNNEST clause.

SQLFluff wants to close the opening bracket on the same indentation it was opened (i.e. where the FROM clause starts), but then the indentation looks weird to me. This is all because the opening bracket at the FROM clause is not encompassing the whole FROM clause - it's only there for the subquery. This is why I call it "faux K&R" because the brackets are not braces, even though we basically pretend they are for our formatting style.

Our choices are:

  1. Accept this weird indentation for any UNNEST statements. We usually use JOIN clauses instead of command lists when joining tables, but UNNEST is the one place we don't do that (and there's a difference between doing it that way).

  2. Switch to this style for UNNEST queries:

SELECT
  *
FROM
  (
    SELECT * FROM a table
  ),
  UNNEST([...])

But know you weren't a fan of that @rviscomi , even if I am as I think it better reflects what the brackets represent.

  1. Persuade SQLFluff to support the other version. I'm still a maintainer on that project (though not really active on it anymore) but know they explicitly removed "hanging indents" support in v2 due to a number of bugs with it, so don't see us persuading them to bring it back.

  2. Stop using the SQLFluff linter. I think it's helped us a lot so not keen on that.

  3. Stay on old versions of SQLFluff and GitHub Supper Linter - NOT a fan of this at all and would prefer to do 4) if we're adamant not to have this.

  4. Find a better SQL Linter which does support our needs. But none do as well as SQLFluff and they will likely have their own opinions here.

What's your thoughts @rviscomi ?

@tunetheweb
Copy link
Member Author

Urgh looks like Super-Linter only includes 2.0.3 and we need a bug fix included in 2.0.4 hence the failures. So we'll need to wait for the next Super-Linter release :-(

Still, would be good to get answers to above for when that happens.

@rviscomi
Copy link
Member

Given that we're pausing the Web Almanac this year and won't be adding any new queries, my temporary preference would be option 5 so we can wait until any better linting options are available. Basically, leave everything as it is and not do any overhaul until later this year when I'm back from leave and we gear up for another Web Almanac edition.

@tunetheweb
Copy link
Member Author

OK but then I'd like to turn off dependabot (at least for GitHub Actions and Python depedencies) to stop getting PRs opened automatically. WDYT?

@rviscomi
Copy link
Member

Yeah that SGTM

@tunetheweb tunetheweb closed this Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants