-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add pyproject.toml support #7247
Conversation
(the code is ready for review I believe, keeping as draft because I need to update the docs) |
ae2d84a
to
a746cc4
Compare
(py35 failures might be related to #7303) |
9c8e884
to
597b5c4
Compare
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.
|
||
Initialization: determining rootdir and inifile | ||
----------------------------------------------- | ||
Initialization: determining rootdir and configfile |
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.
do we want to preserve external links to this target?
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.
I don't really think it is necessary... we've changed section titles in the past. Or is there a way to preserve the old link that I'm not aware of?
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.
Left some comments on the code. I've yet to read to issue to try to understand the ini_options
approach but I get a sense of why it was done...
src/_pytest/config/findpaths.py
Outdated
|
||
result = config.get("tool", {}).get("pytest", {}).get("ini_options", None) | ||
if result is not None: | ||
# convert all scalar values to strings for compatibility with other ini formats |
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.
Should this be recursive? Otherwise I can write e.g. addopts = [true, 10]
.
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.
I don't think so, that's invalid in TOML:
>>> import toml
>>> toml.loads('x = [true, 10]')
...
ValueError: Not a homogeneous array
>>> toml.loads('x = [10, 20]')
{'x': [10, 20]}
Recursive arrays are a possibility:
>>> toml.loads('x = [[10, 20], [30, 40]]')
{'x': [[10, 20], [30, 40]]}
But I would rather be safe here and convert them to string anyway, because that's what we would have gotten if we wrote that in an ini file.
src/_pytest/config/findpaths.py
Outdated
# convert all scalar values to strings for compatibility with other ini formats | ||
# conversion to actual useful values is made by Config._getini | ||
def make_scalar(v): | ||
return v if isinstance(v, (list, tuple)) else str(v) |
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.
Does toml
actually give out both list
and tuple
? Would have assumed it's only one.
What about dicts?
date
s and datetime
s can also be unexpected, but I guess their str
is fine.
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.
You are right, fixed to only test for lists.
Everything else should be converted to strings I think, because that's what we would have gotten if we wrote them in an ini file. The tool.pytest.ini_options
is a bridge to configure pytest using pyproject.toml
files with the legacy options, hopefully we can in the future thinks of better ways to make advantage of the more powerful features of TOML.
src/_pytest/config/findpaths.py
Outdated
|
||
def _get_ini_config_from_pyproject_toml( | ||
path: py.path.local, | ||
) -> Optional[Dict[str, Any]]: |
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.
Basically IIUC we want the Any
here to be Union[None, str, List[str]]
.
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.
I've changed to Union[str, List[str]]
, because I don't think TOML
supports None
?
src/_pytest/config/findpaths.py
Outdated
return None | ||
|
||
|
||
def getcfg(args): |
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.
Would be great if you're able to type-annotate this one 😁 Usually I don't ask this but in this case I think it can really be beneficial for clarifying things.
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.
No worries, feel free to ask anytime... but I just did it, and yikes!
def getcfg(args: List[Union[str, py.path.local]]) -> Tuple[Optional[py.path.local], Optional[py.path.local], Optional[Dict[str, Union[str, List[str]]]]]:
Feel free to suggest how to improve readability here.
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.
I gave it a go there, let me know if you have any suggestions however. 👍
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.
'
Noticed also |
This makes it clear each type of file that it is supported. Also dropped 'config' parameter as it is no longer used.
While setup.cfg might be considered an "inifile", "pyproject.toml" definitely is not.
63dd5ba
to
99ddf36
Compare
Rebased and did further refactorings in |
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.
Great work @nicoddemus, besides adding the new feature, the code in findpaths.py is much cleaner than before.
I agree that the approach taken is a decent way to support pyproject.toml in the midterm without too many consequences for the future or big code changes.
Personally I'd prefer "tool.pytest.ini" over "tool.pytest.ini_options".
I'd also add a short "note" comment in the documentation to explain why it's "pytest.tool.ini_options" and not just "tool.pytest" -- it's seems odd if unexplained.
There's also some type errors that come up when checking with proper py.path.local typing (pytest-dev/py#232), but there are also existing errors so I need to take care of that myself anyway.
This function no longer seems to be necessary
Done! |
@nicoddemus Re. my comment above
Not sure if you missed it or rejected it (which is fine of course). Just seems to me the |
Hi @bluetech Oh sorry, didn't mean to seem dismissive, I thought you had seen this comment: #7247 (comment) That's what we had agreed on the original issue. 😁 |
Fix #1556