Skip to content
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

[WIP] Add support for toml config #3686

Closed

Conversation

tadeoos
Copy link
Contributor

@tadeoos tadeoos commented Jul 15, 2018

Resolves #1556

@tadeoos
Copy link
Contributor Author

tadeoos commented Jul 15, 2018

FYI for some reason I couldn't run tests in py27 - got ImportError: No module named pathlib2. Tests for py36 work fine.
Traceback:

$ tox -e py27,py36 -- testing/test_config.py
...
py27 installed: atomicwrites==1.1.5,attrs==18.1.0,certifi==2018.4.16,chardet==3.0.4,coverage==4.5.1,enum34==1.1.6,funcsigs==1.0.2,hypothesis==3.66.1,idna==2.7,mock==2.0.0,more-itertools==4.2.0,nose==1.3.7,pbr==4.1.0,pluggy==0.6.0,py==1.5.4,pytest==3.6.4.dev149+g0bb29d56.d20180715,requests==2.19.1,six==1.11.0,urllib3==1.23
py27 runtests: PYTHONHASHSEED='4091510751'
py27 runtests: commands[0] | pytest --lsof -ra testing/test_config.py
Traceback (most recent call last):
  File "[truncated]/pytest/.tox/py27/bin/pytest", line 7, in <module>
    from pytest import main
  File "[truncated]/pytest/.tox/py27/lib/python2.7/site-packages/pytest.py", line 9, in <module>
    from _pytest.config import main, UsageError, cmdline, hookspec, hookimpl
  File "[truncated]/pytest/.tox/py27/lib/python2.7/site-packages/_pytest/config/__init__.py", line 17, in <module>
    import _pytest._code
  File "[truncated]/pytest/.tox/py27/lib/python2.7/site-packages/_pytest/_code/__init__.py", line 3, in <module>
    from .code import Code  # noqa
  File "[truncated]/pytest/.tox/py27/lib/python2.7/site-packages/_pytest/_code/code.py", line 11, in <module>
    from _pytest.compat import _PY2, _PY3, PY35, safe_str
  File "[truncated]/pytest/.tox/py27/lib/python2.7/site-packages/_pytest/compat.py", line 46, in <module>
    from pathlib2 import Path
ImportError: No module named pathlib2

@coveralls
Copy link

coveralls commented Jul 15, 2018

Coverage Status

Coverage increased (+0.05%) to 92.52% when pulling 51b3e66 on tadeoos:add-support-for-toml-config into 5f97711 on pytest-dev:features.

@tadeoos tadeoos added type: enhancement new feature or API change, should be merged into features branch topic: config related to config handling, argument parsing and config file labels Jul 15, 2018
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't even pretending to try to support a config file format with completely different semantics correctly

what where you even thinking

@RonnyPfannschmidt
Copy link
Member

now that i finished my first coffee in the morning i'd like to apologize for being overbearingly harsh in my review

as is we cannot accent this pr as the semantics of the .toml format are completely different from .ini files,

as such we can't just use a .toml file like a .ini file, we have to layer support into many parts of pytest starting right at declaration of options as well as going down to using a .toml parser to parse .toml files

@tadeoos
Copy link
Contributor Author

tadeoos commented Jul 16, 2018

Whoa, you're right. I'm actually not so sure what I was thinking -- saw an easy way to fix this and a simple case of .tomlwas parsed correctly, so I got overly optimistic I guess...
Still, I want to implement this feature correctly -- what other parts of pytest do you have in mind? Could you give me an example of a config case that should work in .toml but couldn't in .ini?

@RonnyPfannschmidt
Copy link
Member

@tadeoos the toml format supports nested structures, arrays, date types, and various ways to structure data better,

so supporting it by merely using it like ini files would set things up for worse
an initial version of the support could use a subkey to take in ini alike options,

but in future we really should take a look at taking advantage of the additional capabilities - which means we would need a new api to set up how we use data/configuration keys from the toml file

this is something that will need an actual design document and a few iterations of discussion

@tadeoos
Copy link
Contributor Author

tadeoos commented Jul 16, 2018

@RonnyPfannschmidt, I see.

an initial version of the support could use a subkey to take in ini alike options

What do you think about adding a [tool.pytest.ini] subkey then?

[tools.pytest]

  [tools.pytest.ini]
  # old school inilike configuration opts go here...

this is something that will need an actual design document and a few iterations of discussion

Is there any procedure on creating such a document?

@RonnyPfannschmidt
Copy link
Member

we dont have a procedure in place for that which is unfortunate, i propose writing usage documentation as a starting point

@tadeoos
Copy link
Contributor Author

tadeoos commented Jul 16, 2018

@RonnyPfannschmidt, I'm reading our docs as well as TOML specification and from what I gather, what we need, considering TOML types, is a support for arrays, so this:

[pytest]
filterwarnings =
    error
    ignore::DeprecationWarning

could become:

[tool.pytest]
filterwarnings = [
    'error',
    'ignore::DeprecationWarning'
]

Other then multiple value ones, options mentioned in the docs have values of either str , bool or int type.

The other thing that TOML brings and we could use is, as you mentioned, data structuring, so for example it seems it'd be nice to have something like the following:

[tool.pytest]  # general options go here
maxfail = 3
strict = true
[tool.pytest.reporting]  # and afterward we have sections analogous to the `--help` output
tb = 'long'
showlocals = true
[tool.pytest.coverage]  # plugins could also have their own sections
cov = ['my_package']

How do you feel about this? Is this good enough? Is it a good direction? Am I missing some obvious feature here?

@RonnyPfannschmidt
Copy link
Member

RonnyPfannschmidt commented Jul 16, 2018

@tadeoos those are good points at getting started and developing a initial impression of the file format

another tricky layer will be how to express what types and how to deal with them in the config file - and also the interaction with the ini config handling as we currently do not have a clear idea for a unified api to move torwards

@tadeoos
Copy link
Contributor Author

tadeoos commented Jul 16, 2018

another tricky layer will be how to express what types hand how to deal with them in the config file

Could you elaborate on that? I'm reading the code base but haven't yet managed to grok the whole flow of config initialisation process...

and also the interaction with the ini config handling as we currently do not have a clear idea for a unified api to move torwards

I think I get what you are menaing here... For one the semantic problem in the code I see is that file configs are conceptualised as ini objects (inifile, iniconfig, addini() etc.) A bit hacky option would be to make some layer of abstraction which would make tomlconfig object mimic current ini API. A less hacky option would be to do a lot of refactor (with some good ideas already present of course). Does this make sense? Do you have any thought at all about how such an API could look like?

@RonnyPfannschmidt
Copy link
Member

i only managed to do some intial tinkering, i was going to focus on refactoring first (currently the initialization of the config object is fundamentally broken and it creates unfixable issues for xdist)

im currently not sure how to explain the details myself

@tadeoos tadeoos added the status: help wanted developers would like help from experts on this topic label Jul 18, 2018
@tadeoos tadeoos changed the title Add support for toml config WIP: Add support for toml config Jul 18, 2018
@tadeoos
Copy link
Contributor Author

tadeoos commented Jul 18, 2018

@RonnyPfannschmidt could you elaborate on (or point to) issues for xdist? If we're going to refactor it'd be wise to take then into accout.

@nicoddemus -- do you have any thougths/opinions on the direction this should take?

@tadeoos tadeoos changed the title WIP: Add support for toml config [WIP] Add support for toml config Jul 18, 2018
@RonnyPfannschmidt
Copy link
Member

#1743 captures part of the issue

@RonnyPfannschmidt
Copy link
Member

oh, https://github.com/pytest-dev/pytest/projects/2#card-10815850 has quite many issues related to it

@tadeoos tadeoos force-pushed the add-support-for-toml-config branch from 4fb3b5a to 51b3e66 Compare August 16, 2018 18:26
@nicoddemus
Copy link
Member

Hi @tadeoos,

It has been a long time since it has last seen activity, plus we have made sweeping changes on master to drop Python 2.7 and 3.4 support, so this PR has some conflicts which require attention.

In order to clear up our queue and let us focus on the active PRs, I'm closing this PR for now.

Please don't consider this a rejection of your PR, we just want to get this out of sight until you have the time to tackle this again. If you get around to work on this in the future, please don't hesitate in re-opening this!

Thanks for your work, the team definitely appreciates it!

@nicoddemus nicoddemus closed this Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted developers would like help from experts on this topic topic: config related to config handling, argument parsing and config file type: enhancement new feature or API change, should be merged into features branch
Projects
Development

Successfully merging this pull request may close these issues.

4 participants