Support a new YAML format for template settings #269
Conversation
|
The refactor is done for now. I'm aware that the function names in the settings module are a bit verbose. This is actually somewhere that I think would benefit from a class - used purely as a stateless user defined container, not as any sort of complex OOP design. I know @audreyr has explicitly disallowed class-based design in the core committers guide. I've assumed that prohibition would apply here - am I right? To be clear, all I'm suggesting is an API that looks a bit more like: settings = load_settings_from_file('name/of/file')
context = settings.context()
version = settings.version()Nothing more complex than that - just saving a bit of typing and making the internal layout of the Apart from this question, I think this is ready to go. The hard work of adding new features to actually use the flexibility can then start ;-) |
|
This looks pretty good, though:
Other than that, the settings code looks pretty good to me, though I haven't actually, you know, run it or tested it or anything. 😉 (Also, I didn't look too hard at the tests) |
|
@pfmoore Am i right in thinking that you intentionally left out something like |
| with io.open(name, encoding='utf-8') as f: | ||
| # The JSON file just contains the context. | ||
| context = json.load(f, object_pairs_hook=OrderedDict) | ||
| settings = get_default_settings(version=1) |
There was a problem hiding this comment.
@khorn Here's where a JSON-format file gets a version of 1.
|
@khorn JSON files get a version of 1 in the line I pointed out with a line note. I see where your confusion comes from though. I'll see if I can modify the code to make it clearer what's going on - maybe apply the default in the loading code and not in the get function. And yes, the lack of |
|
@pfmoore Aha. That makes sense now. |
|
@audreyr, @pydanny, @michaeljoseph - any comments on this PR? In particular, I'd like a ruling on whether having a settings object (the alternative API I mentioned above) would be acceptable. This is blocking a number of other changes that depend on extra template settings, so it would be good if we could get it in. |
|
@audreyr is there anything I can do to move this on? Could you comment on whether you prefer the function based API (which I think runs a risk that people will access the settings data structure directly) versus an explicitly encapsulated object API? If you prefer the functional API, I'll look into adding some extra comments to the file and the docs to make it clearer that the settings data has to be treated as completely opaque outside of |
| ctx = settings.get('context', {}) | ||
|
|
||
| # Use the default_context argument to override values in the context | ||
| if default_context: |
There was a problem hiding this comment.
It's not obvious that a method called get_settings would also be responsible for merging context.
I would suggest moving the rest of this method to the calling code in cookiecutter.main
|
@pfmoore, can you articulate why you want to prevent the settings data structure from being accessed directly? I can't see the benefit... I'm 👎 on introducing a class for this (but I do think that stateless classes could be an exception to the rule, where it makes sense) Otherwise, this pull looks good. |
|
The idea behind making the settings opaque is that I want to protect us from needing to update all of the internal code if we introduce a new format. If we access the settings object directly, then the code is closely coupled to the data format, which is bad, as it means that a format change will potentially require extensive code changes. Consider for example if we need to add a new level of nesting in the file format. Ideally, we should be able to do that without having to modify any code outside of The main point of having a class would be to avoid ugly function names like If we don't mind close coupling of the file format with the code, this all goes away. Just do I'll rebase the PR when I get a chance (or is merge considered better practice here?). |
|
🔔 merge is probably safest, but since (probably) no one else has your branch, I'm ok with rebasing. |
|
@michaeljoseph Sigh. I've just had a look at this. It clashes hard with the "extra context" change, which is to "allow for injection of extra context via the Cookiecutter API" (from the history file). But this patch removes the @pydanny and @audreyr both commented in the discussion on PR #260 that there were risks of a clash with this PR, and wanted to consider the changes carefully. It looks like the clash noted above was missed. It certainly never occurred to me that I'm tempted to say we drop this whole PR for now. My motivation for doing it was
But to be honest, I'm not likely to get the time or motivation to push this PR through and then go through the same process with the others as well, any time soon. So rather than leave this hanging, I feel like it would be better to close the PR and leave the idea for someone else to pick up. Comments? |
|
Yeah, agreed, closing. |
|
@pfmoore, I'm really sorry about not being able to properly address this pull request, not to mention some of your other incredible work. We've been very busy, but that's no excuse for not even explaining that we didn't have time to look at your work. I know an apology is only worth words, so let's see what I can do to fix things. (This apology is also directed to @michaeljoseph, the two of you are awesome). Going forward I have a bit of time to spend on cookiecutter. You might have noticed my activity on the project and the release of 0.8.0. If possible we want to push out 0.9.0 quickly, and continue this path of frequent, small, atomic releases. Therefore, what we would like to do is focus an entire release on this feature (config changes) and related pull requests. We would focus 100% on this feature change (only bugfixes allowed otherwise). Personally I would love to see 0.10.0 focused on this effort (1.0 being when we drop support for Python 2.6). This kind of focus is important because some of these new features touch on a lot of things. If you want to work this feature (config changes in 0.10.0), then we'll (me, @michaeljoseph, @audreyr, et al) will focus on speedy, but thoughtful responses. I hope this makes up for everything. One more thing, the same idea of focusing on a particular feature for a release goes for other efforts championed by @michaeljoseph. |
|
@pydanny thanks for the reply. I appreciate your comments. I think the most crucial quiestions for this change are:
I guess the whole point here is, I'm a huge fan of encapsulation - code should know as little as possible about the internals of other parts of the codebase, and all interaction should be via clearly defined interfaces. That doesn't necessarily mean class-based design, but exposing generic data types like dictionaries can make it needlessly hard to ensure encapsulation. At the moment, the only clear boundary that matters to "ordinary users" is the CLI and the spec for writing templates. All other boundaries are only relevant to users of the code - i.e., the core developers. A programming API for cookiecutter implies we need to define further user-level interfaces. All other (internal) encapsulation only matters to the core developers, and people who work on the cookiecutter codebase. But such internal interfaces are useful (IMO) because they act as clear agreements on how closely parts of the code are coupled. At the moment, things like the context dictionary are used throughout the code, and closely couple everything. I'd like to fix that. In the context of this change, we have to decouple the user input from the internal object (that's the point of the change!) and so I'd like to take the opportunity to enforce that decoupling internally at the same time. Enough talk. I don't want this to be an endless debate, so if you can give me a general indication that the above ideas aren't totally unacceptable, and introducing a settings class won't get the change instantly rejected, then I'll put together a new patch (I'll probably just overwrite/rebase this one, so we don't need a new PR) and we can discuss an actual implementation. BUT - can I please ask people to hold back on other changes once the new PR appears - the PR is bound to affect big chunks of the codebase, and it'll be hard to move this forward if I keep having to track conflicting changes hitting master. |
|
Just to clarify my question about the API. The only documented function is Are there 3rd party users of that functionality? Or, to put it another way, if I can no longer support that (or choose to not support it, or to support it differently) with this change, will anyone be affected? |
| context = generate_context( | ||
| context_file=context_file, | ||
| template_settings = get_settings( | ||
| name=context_file, | ||
| default_context=config_dict['default_context'] |
There was a problem hiding this comment.
I think, that instead of replacing the generate_context method here, you should be calling your new method here?
There was a problem hiding this comment.
But the whole point is that I want to isolate the management of the context from the reading of the file. generate_context only returns the context, so how would I retain all of the rest of the data from the settings file? Under the new format, the settings file will contain a lot more than just the context, and should not be managed by the context related functions.
I want one set of functions that reads the settings, and provides an API for client code to read the bits they need. Nothing outside of the settings module should be reading the file, or worrying about which bits of the settings object are the context, or anything like that.
generate_context is a bad function in the new model, because it does both reading of the settings file and managing of the context (merging the user config and extra_context data etc).
I may not have fully understood all of the implications of how the context works (as I said in the discussion about the cookiecutter API, there's a lot of complexity in here that's not used by the command line interface, and I don't know what use cases are as they aren't documented anywhere. Maybe we'll need some sort of compromise solution, but I don't know what until I know what can be refactored, and what is a user-visible API.
| logging.debug('context_file is {0}'.format(context_file)) | ||
|
|
||
| context = generate_context( | ||
| context_file=context_file, | ||
| template_settings = get_settings( |
There was a problem hiding this comment.
I am 👍 on encapsulating Templates as a module/class, that would allow us to write different template definition implementations (like Python?)
There was a problem hiding this comment.
Hmm, I think we're mostly agreeing, but I think you might not be expecting to split things down as finely as I would. I was looking at things like:
- The template settings (context, encoding, top-level object(s), etc)
- The reader (filesystem, git, hg, and we could add more like Python or zipfiles)
- The renderer (Jinja, but simple templates might not need that - also "none" would be useful for binary files)
- The template - but this just contains a settings object and list of files linked to their renderers. The template's purpose is mostly about organising calling functions in the right order.
A reader would produce a template object, and then "rendering" the template would go through its files rendering each in turn with its associated renderer.
At the moment, that's more complexity than we really need. I'd rather introduce the separation as needed - so at the moment we're working on the user interface for the settings, so it makes sense to encapsulate that. When we want to add a new reader, we factor out the reader functionality then. Similarly when we want to add renderers other than Jinja, we refactor that part.
|
Just a note that we'll be digging into this massive conversation tomorrow. I can't wait until 0.10.0 when we get to factor it in. 😄 |
|
Whew. Have a whole day in one place. With broadband internet. So lovely! Okay, to answer @pfmoore's questions:
I don't see why not.
Right now the policy for classes is "classes only for exceptions and in tests". Therefore, is there a way that a dictionary could be used instead of a class? Note: I personally think classes are quite dandy, but I recently assembled a project that used decorators, partials of decorators, and callables as arguments in both the decorators and partials of decorators. So what do I know?
We actually use a number of the cookiecutter functions (i.e. the 'API') outside of cookiecutter. We aren't the only ones. That said, which functions are being used in this way isn't documented. I'll assemble some notes, be it a pull request or a simple gist. From there were can document a formal user backend interface. IMPORTANT As soon as we release 0.9.0 and move towards the 0.10.0 release, we are on feature freeze in order to make @pfmoore's effort to get this pull request (or better yet, a series of smaller pull requests for this issue) into master easier. Small bugfixes for minor issues may be accepted, as well as emergency items (rare). Why? Because I think we all agree we would rather be coding than merging and rebasing. |
|
+1 , I'd like to be able to use yaml instead of json Not being able to comment things in the config (with json) is unfortunate |
|
👍 for YAML config Let me know if I can help move this along. |
This is the initial version of support for a YAML format settings file, to supersede
cookiecutter.json. Before committing, I would like to refactor this further, to make the "internal" format completely encapsulated insettings.pyso that client code need have no knowledge of how the settings are stored. But the PR is sufficiently complete that it's ready for initial review.At the moment, it's only the context data that is in the settings, so the refactoring won't actually impact much, but it establishes the principle that following changes should follow. The main change at this stage will be in the tests, which currently know far too much about how the settings/context is stored.