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

Enhancement - Simplify creation of configuration object #49

Closed
pykong opened this issue Jun 17, 2019 · 5 comments · Fixed by #55
Closed

Enhancement - Simplify creation of configuration object #49

pykong opened this issue Jun 17, 2019 · 5 comments · Fixed by #55

Comments

@pykong
Copy link
Collaborator

pykong commented Jun 17, 2019

This issue relates solely to improving the architecture it does not address any bug or suggests any new functionality.

Creating the configuration object

copier needs to take in account options coming from:

  1. defaults, Like DEFAULT_EXCLUDE and DEFAULT_INCLUDE
  2. files - the copier.yml
  3. user input - CLI args, prompt

It also needs do some basic validation and ensure default values for certain options like, extra_paths defaulting to [] instead of None.

The entirety of the configuration options I herein call the configuration object, albeit this is in the current implementation is not strictly correct as we deal with independent variable not brought under the umbrella of a single dict yet.

The problem

The current approach to creating the configuration object as used in main.copy_local does not strike me as DRY or pythonic.

Consider for example that we duplicated the same command sequence four times:
https://github.com/jpscaletti/copier/blob/18b2c70c3d582a9d3fc241dec7df34c45dcdd1b3/copier/main.py#L203-L216

Also, default values are not defined in a single place:
https://github.com/jpscaletti/copier/blob/18b2c70c3d582a9d3fc241dec7df34c45dcdd1b3/copier/main.py#L132-L133

A cleaner way

While working on SublimeLinter 4 I learned that the problem of creating a single configuration object from different precursor objects is better solved by sequential merger of dicts.

Such a pattern not only allows to define default values in a single place but also easily sets the order of precedence. In this hierarchy of precedence, the default options would reside at the lowest tier, being overridden by options sourced from a copier.yml and those provided at the command line.

conf_obj = {**default_opts, **yaml_opts, **cli_opts}

As an additional advantage, this approach would also enable a much more straightforward validation of the args.

Related: #45

@pykong
Copy link
Collaborator Author

pykong commented Jun 17, 2019

@jpscaletti Can you see this issue I described and would be open to a PR which will give us a simpler more robust code? (Still passing all tests of course ;-) )

@pykong
Copy link
Collaborator Author

pykong commented Jun 18, 2019

Memo:

I really like how pydantic offers both validation and providing default values at the same time.
We could use this to first merge args provided by command line, configuration files and potentially environment variables and then validate against a defined scheme which will at the same time fill out all None values with defaults. Very powerful. Very elegant.

https://github.com/samuelcolvin/pydantic#a-simple-example

This is all done using the modern type hint syntax. Unfortunately, this means we will not be able to use pydantic for copier as we still support py 3.5.

@ghost
Copy link

ghost commented Jun 19, 2019

I agree, this definetely could improve. As you said, here is too much code duplication.

About pydantic and validation, I don't think it is generally applicable enough to justify it.

@pykong
Copy link
Collaborator Author

pykong commented Jun 19, 2019

I agree, this definetely could improve. As you said, here is too much code duplication.

Great. I am working on a PR right now.

About pydantic and validation, I don't think it is generally applicable enough to justify it.

I am referring to our own variables, not any arbitrary setting a user might define.
For example we might check that extra_paths is actually a list of (valid) paths and quiet is a bool.
I consider it best practice to validate user input at a single place before feeding it into the deeper strata of a program. (It makes error handling easier and allows for easier way to provide feedback to the user.

As we are stuck with python 3.5 I need to find an alternative lib to pydantic or implement an own validation routine.

Do you concur that we should perform a basic validation of the user input at least?

@ghost
Copy link

ghost commented Jun 19, 2019 via email

@pykong pykong mentioned this issue Jul 4, 2019
7 tasks
@pykong pykong mentioned this issue Jul 17, 2019
7 tasks
@ghost ghost closed this as completed in #55 Sep 11, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant