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

make list config optional? #213

Closed
mohkale opened this issue Mar 22, 2020 · 9 comments
Closed

make list config optional? #213

mohkale opened this issue Mar 22, 2020 · 9 comments

Comments

@mohkale
Copy link

mohkale commented Mar 22, 2020

I understand the reason for a config being a list, but in my case I don't really need to specify a directive more than once in the config. Because of this, having to prefix every directive with - seems unnecessary.

Thus I suggest allowing simple hash based configs, ergo turning:

- defaults:
    link:
      relink: true
      create: true

- clean: ['~']

- create:
    - ~/downloads
    - ~/multimedia

into:

defaults:
  link:
    relink: true
    create: true

clean: ['~']

create:
  - ~/downloads
  - ~/multimedia

I believe this should be pretty easy to implement in dotbot/cli.py using the following diff.

diff --git a/dotbot/cli.py b/dotbot/cli.py
index 77bd439..d005293 100644
--- a/dotbot/cli.py
+++ b/dotbot/cli.py
@@ -70,8 +70,10 @@ def main():
             log.error('No configuration file specified')
             exit(1)
         tasks = read_config(options.config_file)
-        if not isinstance(tasks, list):
-            raise ReadingError('Configuration file must be a list of tasks')
+        if isinstance(tasks, dict):
+            tasks = [{key: value} for key, value in tasks.items()]
+        elif not isinstance(tasks, list):
+            raise ReadingError('Configuration file must be a list or hash of tasks')
         if options.base_directory:
             base_directory = os.path.abspath(options.base_directory)
         else:
@mohkale
Copy link
Author

mohkale commented Mar 22, 2020

or perhaps it should be implemented wherever the config is read.

@anishathalye
Copy link
Owner

I agree that it would be nice to be able to omit the unnecessary-looking - everywhere. I think it's a little tricky, though. Even from your example, with certain versions of Python (I believe dict was made ordered as part of the 3.8 spec), that will not have the intended effect, because dictionaries are not ordered. That means that the defaults may be processed after the clean and create, for example.

I'd be open to having this as a feature, as long as we could implement it in a way that's consistent with all Python versions that we aim to be compatible with.

@mohkale
Copy link
Author

mohkale commented Mar 22, 2020

So the underlying issue is finding a way to parse the YAML/JSON into an ordered dict? Are you open to switching from pyyaml to another yaml library?

@anishathalye
Copy link
Owner

Yes, and doing it in a clean way that's compatible with all Python versions, and guaranteed by the spec of the YAML parser (not just "it happens to work"). I'd prefer not to switch libraries (are there even any other high-quality Python YAML parsing libraries?)

@mohkale
Copy link
Author

mohkale commented Mar 23, 2020

are there even any other high-quality Python YAML parsing libraries?

None that I've heard of, but I'll look into it when I get a chance.

@StephenBrown2
Copy link

ruamel.yaml is pretty high quality (docs), and maintains order of dicts and such (comments too if dumping) across python versions (i.e. uses collections.OrderedDict instead of default Python 3.6+ dict ordering).

@sandorex
Copy link

I may be missing something but from what i've read on pyyaml#110 it is ordered when reading you should just use an OrderedDict or if you want a drop-in replacement maybe oyaml could work

@anishathalye
Copy link
Owner

From a quick skim, it looks like pyyaml will do the right thing on Python 3.7+, where dictionaries are ordered by the language spec, but I don't think pyyaml will guarantee this behavior with earlier versions of Python.

@anishathalye
Copy link
Owner

This is a tough call, omitting the - will look a little prettier when applicable. But I think we should stick with the current format, where there's a little bit of extra line noise. I don't really want to switch dependencies just for this (e.g. oyaml), and I also don't really want to provide this behavior by the tasks = [tasks] (and sticking with pyyaml), because that's guaranteed to work on Python 3.7+, happens to work on CPython 3.6 (which implemented this before it became part of the language spec), and doesn't work with earlier versions; this could lead to subtle bugs that only show up on certain machines / Python configs. Also, allowing the dictionary format could lead to confusing results if anyone repeats a key name (the config will look reasonable, but do the wrong thing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants