-
-
Notifications
You must be signed in to change notification settings - Fork 653
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
Support Removed warning on configs #3874
Conversation
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #3874 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 179 179
Lines 13782 13800 +18
=========================================
+ Hits 13782 13800 +18
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. One question about the term "deprecated" and/or behavior of it.
src/sqlfluff/core/config.py
Outdated
if k in DEPRECATED_CONFIGS: | ||
formatted_key = ":".join(k) | ||
raise SQLFluffUserError( | ||
f"Config file {file_path} set deprecated config " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typical pattern I've seen for deprecation is to warn the user, then in a future release to remove it or raise an error. Wondering if we want to consider changing the behavior and/or use a term other than "deprecate".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm - good idea. I'm going to change the wording. I think the more "brutal" approach is good because otherwise users may get strange linting results which they look at before any raised warnings.
This is a fairly separable part of #3847, and can be merged separately. It will also simplify the review of that PR to take these changes out of it.
This allows particular config values to be marked as deprecated, prompting the user to remove them if they are set in a config. Optionally it also allows a configurable message to be set, informing the user how to update their config accordingly.