Read config from pyproject.toml #1039
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1039 +/- ##
=======================================
Coverage 95.99% 96.00%
=======================================
Files 123 123
Lines 8157 8176 +19
=======================================
+ Hits 7830 7849 +19
Misses 327 327
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
The PR looks pretty good. I would like to understand the reason for the "__options" suffix in the .toml file. I wonder if there may be other options.
Also, I think we need to update the configuration docs in ocs/source/configuration.rst, which documents how configuration works, what files it looks at, etc.
docs/source/configuration.rst
Outdated
| *[sqlfluff:jinjacontext]*. | ||
|
|
||
| For the `pyproject.toml file`_, all valid sections start with `tool.sqlfluff` | ||
| and subsections are delimited by a dot and suffixed with *__options*. |
There was a problem hiding this comment.
You mentioned the __options suffix in the PR description. Can you give an example of the kind of conflict you're avoiding by doing this?
If I understand correctly, could the toml reader skip sections that don't start with tools.sqlfluff or sqlfluff?
There was a problem hiding this comment.
So a TOML is basically a nested object/dictionary and the keys would collide. The following code raises an error
import toml
content = """
[tool.sqlfluff]
templater = "jinja"
[tool.sqlfluff.templater]
unwrap_wrapped_queries = true
"""
toml.loads(content)The error is:
What? templater already exists?{'templater': 'jinja'} (line 5 column 1 char 38)
If I understand correctly, could the toml reader skip sections that don't start with tools.sqlfluff or sqlfluff?
Yes, the other sections are ignored. The convention I believe is to put every set of configurations for my_tool under [tool.my_tool*] sections.
| the first is a tuple of paths, the second is the value at that path. | ||
| """ | ||
| config = toml.load(fpath) | ||
| tool = config.get("tool", {}).get("sqlfluff", {}) |
There was a problem hiding this comment.
Now that you replaced __options with the tool.sqlfluff.core technique, do you need to move or place the core values to match the structure of the other config file types?
In other words, have you verified that SQLFluff correctly uses the values from the core section?
[tool.sqlfluff.core]
templater = "jinja"
sql_file_exts = [
".sql",
".sql.j2",
".dml",
".ddl",
]
There was a problem hiding this comment.
@barrywhart I added a test case with
[tool.sqlfluff.core]
testing_int = 5
testing_bar = 7.698
testing_bool = false
testing_arr = [ "a", "b", "c" ]
testing_inline_table = { x = 1 }
[tool.sqlfluff.bar]
foo = "foobar"
[tool.sqlfluff.fnarr.fnarr]
foo = "foobar"which gets collected into the following dict:
{
"core": {
"testing_int": 5,
"testing_bar": 7.698,
"testing_bool": False,
"testing_arr": ["a", "b", "c"],
"testing_inline_table": {"x": 1},
},
"bar": {"foo": "foobar"},
"fnarr": {"fnarr": {"foo": "foobar"}},
}does it look right?
There was a problem hiding this comment.
TBH, I'd feel much better if you could do some manual tests to confirm the settings in the TOML file are being read correctly and actually affecting the runtime behavior of SQLFluff. There are sometimes limits to the effectiveness of automated tests and manual review, and I think this is one of those times.
There was a problem hiding this comment.
@barrywhart That makes a lot of sense. I created this repo to test some configurations. Let me know if you think there are edge cases I could add.
There was a problem hiding this comment.
I tested with your example repo. Works great! I also added a simple test with templating. This is based on the Variable Templating example from the SQLFluff docs.
test.sql:
SELECT {{ num_things }} FROM {{ tbl_name }} WHERE id > 10 LIMIT 5
Additions to pyproject.toml:
[tool.sqlfluff.core]
templater = "jinja"
[tool.sqlfluff.templater.jinja.context]
num_things=456
tbl_name="my_table"
There was a problem hiding this comment.
I added that templated case to the example repo 😄.
This PR adds support for
pyproject.tomlas a configuration source.It would help keeping all configuration for a Python project in a single file, since this file is used by other major Python projects like black and pytest.
Some implementation details:
Takes precedence over
.sqlfluff.coresection lives in[tool.sqlfluff.core].Closes #890