L029: Optionally check quoted identifiers in addition to naked identifiers #1775
Conversation
|
Not sure I agree with this change. The whole point of reserved words are that they can’t be used unless quoted. While I agree that might be best practice I don’t think this should be added to rule L029 by default. It should either be a configurable extra, or a whole new rule IMHO so it can be enabled/disabled independently. |
|
@tunetheweb I can set it up as configurable. Something like quoted_identifiers_policy to match the existing unquoted_identifiers_policy? |
|
Maybe. Will it offer the same options? Or would should it just be on/off or true/false? In which case something like But can you explain a little more the rational why you think this shouldn’t be used? Is it invalid in T-SQL? Or is there some other reason? |
|
I think it's a good general rule to avoid using reserved or unreserved keywords for other things, even when bracketed/quoted. It reduces ambiguity. The specific case that I really wish to avoid is creating a view or table that uses a keyword as a column. That means mandated bracketing/quoting whenever we reference that column if it's a reserved keyword, and lack of clarity at the least when it's an unreserved keyword. |
Codecov Report
@@ Coverage Diff @@
## main #1775 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 138 138
Lines 9792 9794 +2
=========================================
+ Hits 9792 9794 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
One small suggestion, but looks pretty good to me.
@tunetheweb: Would you like to review as well? Holding off approving until I hear from you.
Won’t be near a computer for next few days so happy to go with your approval. One slight nit from a quick look on phone is all test cases are t-sql, which uses different quotes than most. Might be good to have one ANSI I’ve too. But other than that great set of test cases. |
|
@jpers36: The other reviewer suggested adding at least one test case that doesn't use TSQL. Could you add an ANSI test case to address that? This raises another question -- are there any databases where quotes are more than one character long? If so, code like this wouldn't work correctly: |
|
@barrywhart regarding the single character questions, I just investigated all currently-supported databases and confirmed they all use single characters for quote/delimited identifiers. Working on the rest of the recommendations. |
There was a problem hiding this comment.
Think this is good to merge. @barrywhart anything holding it up from your end?
tunetheweb
changed the title
Fixes #1747
This PR adjusts L029 to optionally check quoted identifiers in addition to naked identifiers.
In order to do this, we are adding a new configuration keyword, quoted_identifiers_policy, which represents the same thing as unquoted_identifiers_policy but for quoted identifiers.
The options for quoted_identifiers_policy are the same as unquoted_identifiers_policy with the addition of none, representing no checks for quoted identifiers.
We check quoted identifiers by stripping off the first and last character. If we have a case where this shortcut would fail, we'll need to tweak the code before merging.
Changes:
+quoted_identifiers_policy default of none to default_config.cfg
+quoted_identifiers_policy description in config_info.py
L014.py: Rename unquoted_ids_policy_applicable function to identifiers_policy_applicable in order to reflect change in functionality
L029.py: Reflect renaming of unquoted_ids_policy_applicable function; add quoted_identifiers_policy as a configuration keyword; adjust rule to check quoted identifiers based on the config setting.
L029.yml: Rename test cases to be more descriptive. Add quoted identifier test cases.