Skip to content

Conversation

PhilipTrauner
Copy link
Contributor

@PhilipTrauner PhilipTrauner commented Feb 17, 2020

This should mainly serve as a basis for discussion, as it's essentially just a quick hack to get the functionality in. With that said, here's why I think that a per-file linting toggle would be beneficial:

  • psql scripts can contain syntax constructs, that aren't valid SQL (e.g. \set and the respective psql variable usage :'variable')
  • pglast is currently stuck on Postgres 10, which prevents the usage of things like generated columns.

With my reasoning out of the way, there are still a couple questions left that need answering:

  • How should stdin output be handled?
  • Should this be handled as part of the per-file plugin configuration logic?
  • Should it be squabble-disable as a single line comment, ... or a multi-line comment? So many possibilities!
  • Is this a good idea in the first place?

With kind regards,
Philip Trauner

@erik
Copy link
Owner

erik commented Feb 17, 2020

Completely on board with this idea, thank you for sketching it out!

How should stdin output be handled?

IMO, cat foo.sql | squabble should operate the same as squabble foo.sql, so I think it makes sense to have the preamble support both. The per-file config is evaluated before the file is actually sent through pglast, so we should be able to treat stdin and files the same way without worrying about syntax errors.

Should this be handled as part of the pre-file plugin configuration logic?

I like the idea of tying this in with the existing configuration logic just to keep things consistent.

Looking at ways other projects do this:

# pylint: skip-file 
# pylint: disable-all
/* eslint-disable */
// CHECKSTYLE:OFF
# rubocop:disable all

In retrospect, I really should have made the enable/disable flags something like -- squabble-disable: RuleName to make it more obvious what's going on. I think I'll add support for those in a separate change.

I think your proposed -- squabble-disable would work! So we'd then have:

-- squabble-disable              /* disable the linter entirely */
-- squabble-disable: SomeRule    /* disable a specific rule */
-- squabble-enable: AnotherRule  /* enable a specific rule */

Should it be squabble-disable as a single line comment, ... or a multi-line comment? So many possibilities!

Single line comment is definitely the easier route. As far as I remember pglast doesn't actually retain comment information in its syntax tree, so we'd have to roll our own mini parser to actually handle multiline comments correctly

Is this a good idea in the first place?

Yes!

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.

2 participants