Avoid overwriting settings by storing them separately on the configuration context

[kirberich]

I've gone ahead and changed the way ConfigurationContext stores its cache, manifest, updater and versioner so it doesn't have to overwrite settings variables, which can cause a lot of confusion when debugging ("Why is my boolean variable suddenly an instance of some class I've never header of?").

I had to move the support for absolute paths in manifests (I think?) from scripts.py to the ConfigurationContext, which I think is the more correct behaviour, but I don't completely understand what that code does, so it'd be good if you could double-check it.

I need this to be able to update the settings handling in django-assets, I hope this doesn't affect any of the other framework integrations.

To make this run on my travis I also had to completely take apart the travis configuration. A lot of requirements seemed to be not necessary anymore to make the tests pass, so I've removed them, it's quite possible though that they're needed for local development that I didn't touch. Let me know if that's the case and I'll try and restore them.

Some of them caused trouble, libsass for example needs to be compiled (which travis doesn't like on the new docker containers).

[kirberich]

I'm only now seeing the appveyor thing and I'm not sure what that is, let me know if you want me to look into it please!

[miracle2k]

Thanks, this looks really good. Fixing this particular issue with the settings is something I wanted to do for a long time.

I've sort of given up on on making the Travis builds work, because I've run repeatedly into issues that seemed to be related to environment changes, and trying to fix them via guessing what adjustments to make to travis.yml is a huge pain. It seems you fixed them, which is amazing. AppVeyor I used at one time to test on Windows.

About the code code you moved from the CLI module to the Environment - here is what's going on:

• The API of FileManifest is this: If the string is "file:/srv/foo", write to /srv/foo, but if it is "file:foo", write to ${envdir}/foo.
• The API of the --manifest option in the CLI was supposed to support an extended format. --manifest foo is supposed to write to $(cwd)/foo - i.e. it is not necessary to provide a format prefix like "file" or "json".

Before, settings "manifest" to "foo" without any prefix was an error. With your change, the manifest option supports the same behaviour as was the case on the command line. Which is not a big problem, but I think I don't like the change.

• I have no problem forcing the developer to be explicit about the manifest they want.
• We might want to change the default manifest written by the CLI to say "json". Changing the CLI behaviour is one thing, changing the behaviour of a core builtin class is quite different.
• The thinking behind these strings is such that the path is the optional part. That is can say the manifest should be "file", and only if I want to change the default file (.webassets-manifest) do I write file:foo.

You say this change what necessary, but why?

[kirberich]

I'm cool with leaving the core API as it is and moving that special format behaviour back to the CLI, I wasn't sure how to do it nicely though - before my change the manifest was instantiated in the CLI, but now it's just the settings value that get passed the along, so the value error only occurs when actually accessing the manifest.

Should I just remove the error handling code from the environment and make the CLI default to json then?

[miracle2k]

My suggestion would be that the code in script.py can remain as is, that is, get_manifest is used to validate the value provided by the user, and the manifest instance is then assigned to environment.manifest. That is, the property does not receive a string value in this case, which should be a valid use case.

Admittedly I had issues in the past with a custom settings backend that only support string values, but that strikes me as a separate problem; env.manifest = some_instance seems like it should be supported.

[kirberich]

I can try and come up with a way of allowing to pass an instance of the manifest to the environment, but it is a bit awkward: passing in an instance would bypass the settings without changing them, resulting in a mismatch between setting and instance, making confusing-to-debug problems possible.

If the setters are instead treated as interfaces to the settings variables then the settings are the single source of truth and everything derives from them.

Side note: I keep concentrating on the manifest, but I'm guessing any behaviour will need to be mirrored for the cache, updater and versioner, to keep the interfaces symmetric.

[miracle2k]

Maybe I'm not thinking this through completely, but in my mind:

env.manifest = instanceOfAManifest()

would write this instance to the settings backend. The manifest property getter would realize that the settings backend already stores an instance, rather than a string, and wouldn't need to transform it. In fact, my suspicion is that the get_manifest (or get_cache etc helpers) already pass through an instance, so no further change would be required.

We would still accomplish the goal of not replacing stuff in the settings object without the users knowledge. It would only happen if the user purposefully wants to us an instance.

Whether we should use this ability to assign an instance the user-facing parts of webassets itself (like the script interface) is a different question - the problem I ran into a while ago was that I wanted to use a string-only settings backend. But it might also be a question for another time.

For now, the current behaviour could be maintained as-is, and there is a single instance in the CLI interface where webassets wants to use a manifest instance.