[WIP] Initial toml support

[RazerM]

Missing docstrings and tests at the moment. If this looks like the right idea I can start adding those.

Example pyproject.toml:

[tool.coverage.run]
branch = true
source = ["outcome", "tests/"]

[tool.coverage.report]
precision = 1
exclude_lines = [
    "pragma: no cover",
    "abc.abstractmethod",
]

Resolves #664

[nedbat]

@RazerM Thanks for all of this! :)

I'm reluctant to add this code to the heart of coverage.py though, partly because of the requirement on the toml library. I have another idea: this can be a separate package, implementing a configurer plugin. The change I'd need to make in coverage.py is to find plugins via entry points rather than just explicit configuration.

What would you think?

[RazerM]

As you suggested in #664, I made it so that toml files aren't loaded if toml isn't installed, so there's no hard requirement there.

As a user of coverage, I think I'd prefer to add e.g. coverage[toml] to my dev-requirements.txt and have it Just Work™. I suppose that's a fairly weak argument given that it'd be just as easy to add a coverage-config-toml package or whatever to my requirements. Although that library could also be added as an extra to coverage.

[nedbat]

I see your point about the convenience, and an extras would work.

I appreciate the adapter class you've made here so that the toml file can be read like an ini, and I definitely appreciate you following the conventions in the rest of the code (backwards.py etc).

I've haven't looked into toml, but I'm wondering if there's a simpler way to get configuration from a toml file. The adapter class seems a bit bulky for the conceptual problem to be solved?

[RazerM]

TOMLConfigParser.read accepts multiple filenames for compatibility with HandyConfigParser, even though that functionality isn't used.

toml.load gives:

{
    'tool': {
        'coverage': {
            'run': {'branch': True, 'source': ['outcome', 'tests/']},
            'report': {
                'precision': 1,
                'exclude_lines': ['pragma: no cover', 'abc.abstractmethod']
            }
        }
    }
}

So the get<type> methods are mostly doing an isinstance check for the already deserialized value (and it occurs to me that TOMLConfigParser.getfloat will fail for integers at the moment).

To use something else we'd need an alternate path to reading and checking types from CONFIG_FILE_OPTIONS.

[nedbat]

@RazerM OK, I'm warming to the idea of this. I would put the TomlConfigParser (not TOMLConfigParser...) class into a separate file. I guess that would mean replace_environment_vars would go into misc.py.

How does that sound?

[nedbat]

BTW, I just refactored the environment variable substitution....

[RazerM]

I should be able to write tests sometime within the next week.

[ptink]

I'd very interested in helping getting this over the line- getting people to adopt pyproject.toml is really hard when a ubiquitous library like coverage doesn't support it yet :) @RazerM if your time is scarce at the moment I could help with writing tests?

[RazerM]

@ptink Help would be great! I think we can copy toml_config.py to do the same tests for pyproject.toml. We may need some unit tests for the TomlConfigParser class too.

[RazerM]

FYI @ptink I have started writing some tests.

@nedbat one problem I've come across is environment variable substitution for non-strings (i.e. integers and booleans):

[tool.coverage.run]
branch = "$OKAY"

Currently, I have

def getboolean(self, section, option):
    value = self.get(section, option)
    if not isinstance(value, bool):
        raise ValueError(
            'Option {!r} in section {!r} is not a boolean: {!r}'
            .format(option, section, value))
    return value

So the options are:

1. disable substitution unless we expect a string.
2. try to coerce strings into the type we expected.
3. try to coerce, but only if variable substitution occurs.

Option 2 is the most user friendly, but it means users can use branch = "yes" instead of using a TOML boolean, which would look strange.

[nedbat]

Maybe I'm missing a detail. Why would coercing a string to the type mean that "yes" would be true? Because it's a non-empty string, and "no" would also be true? That would be the strange case. I'm not sure what value you had for $OKAY that you wanted to support?

Would branch = $OKAY work, and be different than branch = "$OKAY"?

[RazerM]

Maybe coerce was the wrong word. I'll try explain what I meant.

As it stands, ConfigParser loads everything as a string. Coverage will call getboolean('run', 'branch'), and HandyConfigParser loads the string for that key, substitutes any environment variables, then convert's 'yes', 'no' etc. to a boolean.

With TOML, by the time our class gets a value, it's already whatever type was in the TOML file:

>>> import toml
>>> toml.loads("""
... [run]
... branch = true
... """)
{'run': {'branch': True}}
>>> toml.loads("""
... [run]
... branch = "false"
... """)
{'run': {'branch': 'false'}}

As it stands in this PR, the getboolean function I showed previously just raises an error if it's a string. If we want to support environment variables, we'll have to accept strings, substitute environment variables, and convert the value into a boolean (e.g. we could accept the same 1/yes/true/on values that ConfigParser does).

[daneah]

Hey folks! Is there anything I can do to help this change along? I've been investing some time trying to unify to one of setup.cfg or pyproject.toml for a project of ours, but because black won't seek to support setup.cfg (AFAICT) the resolution I'd love to see is a release with this change 😄

[daneah]

@nedbat or @RazerM, is there anything I can do to help?

[nedbat]

@daneah Do you have an opinion about the environment variable concern? I suppose the simplest thing for now would be to not support environment substitutions in pyproject.toml files, at least for non-string values.

[nedbat]

I think this clause would be better in backward.py. I'd prefer something like:

try:
    import toml
except ImportError:
    toml = None

and then later using "if toml:" to check for toml support.

[daneah]

@nedbat I haven't used environment variable substitution in the past, and I'm not sure I was even aware it was supported until I read this conversation! That probably means I'm not a good one to provide an opinion, but maybe the data point is helpful. It does seem like the kind of thing that could be iterated on and enhanced in the future if TOML support for environment variables starts out with only strings.

[RazerM]

I'll resume the work on this without environment variable support for non-string values.

[codecov-io]

Codecov Report

Merging #699 into master will decrease coverage by 9.79%.
The diff coverage is 59.11%.

@@            Coverage Diff            @@
##           master     #699     +/-   ##
=========================================
- Coverage   92.23%   82.44%   -9.8%     
=========================================
  Files          85       86      +1     
  Lines       11335    11529    +194     
  Branches     1148     1179     +31     
=========================================
- Hits        10455     9505    -950     
- Misses        739     1837   +1098     
- Partials      141      187     +46

Impacted Files	Coverage Δ
coverage/cmdline.py	89.7% <ø> (-0.67%)	⬇️
tests/test_config.py	92.65% <53.33%> (-7.35%)	⬇️
coverage/tomlconfig.py	57.75% <57.75%> (ø)
coverage/config.py	97.56% <80%> (-0.78%)	⬇️
coverage/backward.py	95.52% <80%> (-1.26%)	⬇️
tests/plugin2.py	0% <0%> (-62.97%)	⬇️
tests/test_plugins.py	44.52% <0%> (-54.68%)	⬇️
tests/test_context.py	60.46% <0%> (-39.54%)	⬇️
tests/test_api.py	59.57% <0%> (-36.72%)	⬇️
tests/test_oddball.py	66.93% <0%> (-33.07%)	⬇️
... and 32 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8c5066...e54acb9. Read the comment docs.

[RazerM]

@nedbat I think this is ready for review. If there should be more in the testing or documentation department let me know.

One possible UX problem right now is if the user passes --rcfile coverage.toml, we'd silently ignore it when toml isn't installed. Should we warn the user if they explicitly pass a .toml filename and don't have the toml extra installed?

We could have a similar problem if users add configuration to their pyproject.toml and don't realise it isn't being used. Either we do nothing in that case or print a message that the file is being ignored when it exists but we didn't ready any standard configuration files.

[nedbat]

Tests are failing on Travis and AppVeyor. Do they pass for you locally?

[RazerM]

@nedbat I must have broke something when I rebased, I’ll have a look after work.

[nedbat]

Thanks for persisting with this. We are very close to being done. I can squash and merge as is, and make a few changes myself, unless you want to take care of them.

[nedbat]

We don't have a case where our_file is True, so we could remove a little bit of code here.

[nedbat]

I'm curious about the "coverage.toml" case. Is that something we need to support at all? I thought TOML support was so that we could piggyback on pyproject.toml? I guess a few of my comments would go the other way if you felt it was important to allow for "coverage.toml".

[RazerM]

@nedbat It supports --rcfile=coverage.toml. The only default we load is pyproject.toml. I haven't used --rcfile in my own projects, but I would like to be able to use toml if I did.

My motivation for the PR was to be able to use TOML since I prefer the syntax. So for me, the standard pyproject.toml file is just an additional benefit.

One example is a new developer being able to see which values are lists at first glance:

[tool.coverage.run]
source = ["mypackage"]

vs

[coverage:run]
source = mypackage

[nedbat]

@RazerM What's left to do here? Issues raised so far:

1. Only string values (including lists of strings) can have environment variable substitution. (BTW: would it be crazy to read the entire file as a string, substitute in that entire string, and then read it as TOML?)
2. If someone creates a .toml configuration file, but doesn't install the TOML support, there's no warning.

I think we're OK with merging without changing them.

Some things I can do after merging:

1. Change the RuntimeError to a CoverageError
2. Add tests of the no-toml-available case, which will require some experimentation to get right.
3. Think about whether there's a simple way to warn people if they've created a TOML config, but don't have TOML support installed.

[nedbat]

Oops, and docstrings :)

[nedbat]

I tried the crazy idea of substituting environment variables on the whole file contents at once, and the tests all pass, so maybe it's not so crazy?

diff --git a/coverage/tomlconfig.py b/coverage/tomlconfig.py
index 0d084603..38f26644 100644
--- a/coverage/tomlconfig.py
+++ b/coverage/tomlconfig.py
@@ -3,7 +3,7 @@
 import re
 
 from coverage import env
-from coverage.backward import configparser, path_types, string_class, toml
+from coverage.backward import configparser, path_types, toml
 from coverage.misc import CoverageException, substitute_variables
 
 
@@ -29,7 +29,9 @@ def read(self, filenames):
         for filename in filenames:
             try:
                 with io.open(filename, encoding='utf-8') as fp:
-                    self._data.append(toml.load(fp))
+                    toml_data = fp.read()
+                    toml_data = substitute_variables(toml_data, os.environ)
+                    self._data.append(toml.loads(toml_data))
             except IOError:
                 continue
             except toml.TomlDecodeError as err:
@@ -89,8 +91,6 @@ def get(self, section, option):
                     value = section[option]
                 except KeyError:
                     continue
-                if isinstance(value, string_class):
-                    value = substitute_variables(value, os.environ)
                 return value
         if not found_section:
             raise configparser.NoSectionError(section)
@@ -110,9 +110,6 @@ def getlist(self, section, option):
             raise ValueError(
                 'Option {!r} in section {!r} is not a list: {!r}'
                 .format(option, section, values))
-        for i, value in enumerate(values):
-            if isinstance(value, string_class):
-                values[i] = substitute_variables(value, os.environ)
         return values
 
     def getregexlist(self, section, option):

[nedbat]

Also, all the errors we raise in tomlconfig.py should derive from CoverageException, so that users don't get tracebacks if the error is reported on the command line.

[pawamoy]

Think about whether there's a simple way to warn people if they've created a TOML config, but don't have TOML support installed.

@RazerM previously proposed this:

print a message that the file is being ignored when it exists but we didn't read any standard configuration files

So, if coveragepy finds no default configuration file except pyproject.toml, but toml is not installed, print a message. But I think it would trigger a false positive for users who have no coverage configuration but still a pyproject.toml and no toml package installed. The message should only be printed if there is coverage configuration in pyproject.toml, meaning... we must read pyproject.toml to know if we should read pyproject.toml hahaha...

[nedbat]

For others who have reacted on this issue: Would you use TOML support for a dedicated coverage configuration file (coverage.toml), or are you interested in this just for writing coverage configuration into a larger file like pyproject.toml?

React with thumbs-up 👍 for dedicated coverage.toml file.
React with thumbs-down 👎 for only larger toml file.

/cc @pawamoy @daneah @tpansino @DolanMaize @ptink @ipmb @sweenu

[nedbat]

@pawamoy For the "no TOML installed" warning, riffing on the "read the whole TOML file as a string" idea, we could just search that string for "[tool.coverage." and warn if it's found.

[davidism]

Supporting separate coverage.toml files means that we can opt in to using TOML for configuration while opting out of any behaviors currently caused by the presence of a pyproject.toml file. For example, there was an issue earlier this year with how pip treated editable installs when pyproject was detected, and while that's fixed it still has somewhat different behavior than editable installs without pyproject. towncrier recently added a --config option for this reason.

[pawamoy]

I voted with a thumbs down (only larger toml file) because I'm only interested in putting my config in pyproject.toml. However I'm absolutely not against having the option to specify another TOML file to read the config from. If I understand correctly, nobody asks coveragepy to support a dedicated coverage.toml file (if this is what you meant @nedbat). As @RazerM said:

The only default we load is pyproject.toml

To summarize: I'm only interested in pyproject.toml, though others (and maybe myself later) find the ability to do --rcfile=path/to/my_coverage.toml useful, if not essential (as stated by @davidism)

[nedbat]

@RazerM I'm going to clean this up and merge it, thanks!

[nedbat]

I'm closing this: these changes are all part of #865 now. @RazerM Thanks so much for persisting with this, sorry it took so long!