Separate out wpt-specific parts of config from wptserve #10850
Conversation
jgraham
force-pushed
the
config_fix
branch
4 times, most recently
from
a6968aa
to
9f77a66
Compare
tools/wptserve/wptserve/config.py
Outdated
| @property | ||
| def paths(self): | ||
| return {"doc_root": self.doc_root, | ||
| "ws_doc_root": self.ws_doc_root} |
There was a problem hiding this comment.
You removed ws_doc_root above?
(We don't seem to have tests for this, though idk what we can do beyond copying the method and asserting it's equal, but even that badness would catch it throwing AttributeError.)
| class Config(config.Config): | ||
| """serve config | ||
|
|
||
| this subclasses wptserve.config.Config to add serve config options""" | ||
|
|
||
| _default = { |
There was a problem hiding this comment.
This to me seems like a big loss in discoverability compared with having it in the JSON file, as it makes it much harder to know what the format is if you want to modify anything.
There was a problem hiding this comment.
OK, but that's a problem that almost no one has because almost no one modifies this. I think you're right and I"m happy to address it in a followup e.g. by changing the documentation or adding a config.sample.json file. However actually using a file would require inventing some substitution syntax to get the right root directory, which seems like even more complexity, which is why I changed things.
This moves the parts of the Config class that are specific to wpt into serve.Config, so that wptserve can be used independently of the rest of wpt. It also moves the sslutils module into wptserve since this is required for the ssl configuration in wptserve and isn't useful independently. As part of this, the config.default.json file is removed in favour of a default configuration defined in the class itself.
I need to land something like this upstream; just creating this PR to ensure that all the tests pass in Travis.
This change is