fix #374 - initialize configuration for toml integration #377
Conversation
RonnyPfannschmidt
changed the title
| defn = __import__("toml").load(strm) | ||
| return cls()._load(defn.get("tool", {})["setuptools_scm"]) | ||
| section = defn.get("tool", {})["setuptools_scm"] | ||
| return cls(**section) |
There was a problem hiding this comment.
The danger (and benefit) to this approach is it couples the class interface to the configuration schema. It's now invalid to have any values in the configuration that aren't supported by the class, which means now users must be much more sensitive to the version of setuptools_scm used, making it more difficult to be forward-compatible (supplying a value that is used if supported but ignored otherwise). It was my intention to be suitably lenient, acknowledging the risk that a user might supply the wrong key (with a typo or something) and that typo would fail silently.
If you want to go with this approach, I'd recommend allowing **kwargs to Configuration() and (if desired) warn if unexpected keyword arguments are found.
There was a problem hiding this comment.
good point, i#ll think about how to layer it in correctly
| root=".", | ||
| version_scheme="guess-next-dev", | ||
| local_scheme="node-and-date", | ||
| version_scheme=DEFAULT_VERSION_SCHEME, |
There was a problem hiding this comment.
These defaults feel duplicative to me. How about instead:
def get_version(*args, **kwargs):
return _get_version(Configuration(*args, **kwargs))or get_version = compose(_get_version, Configuration) (where compose is something like this)?
There was a problem hiding this comment.
unfortunately the argument order is different compared to Configuration
(this is a general oversight)
i do wonder if perhaps a breaking release should be done about this and the arguments (with some more change accumulated on top)
There was a problem hiding this comment.
Is the constructor for Configuration part of a public interface? I'd have imagined that was private, so could adapt to match get_version (maybe; I haven't tried) without affecting compatibility.
There was a problem hiding this comment.
In setuptools_scm I have made some mistakes about the semantics of public vs private, it may be necessary to take time for a detailed workout on that
There was a problem hiding this comment.
No problem. Happy to proceed with it as is.
|
|
||
|
|
||
| def test_pyproject_support(tmpdir, monkeypatch): | ||
| pytest.importorskip("toml") |
There was a problem hiding this comment.
hmm. I'd prefer to make toml a hard requirement (for tests). Are there circumstances where toml couldn't be required for tests? I'd like to imagine setuptools_scm moving to hard-requiring toml sooner than later.
#374 indicates a need for a followup with unifying configuration initialization at the right place,
followup in #393