Conversation
- implement class TOMLFileConfigLoader - implement corresponding test case - add toml to requires list in setup.py
|
Looks like the failed tests are unrelated to your changes. There's another PR with just doc enhancements that fails too. |
|
can it be related to jupyter/notebook#5986? |
|
It's great to see an addition of a format for traitlets, but I think it would make more sense to not make it a hard dependency. TOMLFileConfigLoader could wrap the currently top-level "import toml" in a try-except to not force the installation of toml for anyone who won't be using that feature, but giving a helpful error message for those wanting to use it to install |
|
I have added the optional import of toml. The only thing which is suboptimal now is the testing of optional dependency (it is a bit ugly). I'd appreciate if someone has a better idea of testing implementation (or suggestion on optional dependcy incorporation) |
|
Normally in these situations, I try to isolate this sort of "environment dependent code" in its own module so you don't have to do any |
| import sys | ||
| import json | ||
| try: | ||
| import toml |
There was a problem hiding this comment.
we should likely now prefer py3.11+ stdlib's tomllib and fall back to tomli
| extras_require = setuptools_args['extras_require'] = { | ||
| 'test': ['pytest'], | ||
| 'test': ['pytest', 'toml'], | ||
| 'with-toml': ['toml'], |
There was a problem hiding this comment.
would probably just add tomli ; python_version < 3.11 to install_requires instead
| return toml.load(f) | ||
|
|
||
| def _convert_to_config(self, dictionary): | ||
| if 'version' in dictionary: |
There was a problem hiding this comment.
i don't recall having seen version on any of the other sources, and don't see why we'd want to add that constraint here unless it was added to all of them, which would be a major version bump
6 participants
Fixes #648