added methods to load config from a file #199
Conversation
Archelyst
force-pushed
the
improved_config
branch
from
ab88c7a to
04798cb
Compare
|
Hi @Tim-Erwin ! |
|
|
||
| class Config(dict): | ||
| def __init__(self, defaults=None): | ||
| dict.__init__(self, defaults or {}) |
There was a problem hiding this comment.
Maybe it's better to use super here?
There was a problem hiding this comment.
Idk, not really a difference, I'd say. Can change it.
There was a problem hiding this comment.
I don't think that built-in dict will inherit a lot of other stuff, but please go with super here just in case
| self.ROUTER_CACHE_SIZE = 1024 | ||
|
|
||
| def __getattr__(self, attr): | ||
| return self[attr] |
There was a problem hiding this comment.
according to https://docs.python.org/3/reference/datamodel.html#object.__getattr__ this method should raise AttributeError, but in this case it will raise KeyError instead
| module.__file__ = filename | ||
| try: | ||
| with open(filename) as config_file: | ||
| exec(compile(config_file.read(), filename, 'exec'), |
There was a problem hiding this comment.
I understand this is a bit paranoid, but is it really safe to execute compiled third-party file? But in any case I can't propose better approach
There was a problem hiding this comment.
Well, it's not really third-party, you're including your own file. It's quite common to use a python module as configuration. So I'd call that a well-proven procedure :)
There was a problem hiding this comment.
you could just dynamically import it with importlib if you want to
There was a problem hiding this comment.
I tried using importlib, however it fails when the config file does not have the .py suffix. Anyway, whether we are loading with importlib or with compile/exec does not really matter, in both cases we're executing python code. Or is there any benefit I'm missing?
|
|
||
| :param obj: an object | ||
| """ | ||
| for key in dir(obj): |
There was a problem hiding this comment.
maybe it would be faster to make a list comprehension with filter for removing keys which starts with __ or _? I don't have a performance test, but I think it would be faster then go through every key
There was a problem hiding this comment.
And then issue self.update()? I'm modifying a dict in the loop, not creating one.
There was a problem hiding this comment.
By the way: the isupper() is quite intentional instead of checking for _ because it allows to have some logic in your config file using non-upper variables without messing up your final config.
There was a problem hiding this comment.
sorry about disturbing, doesn't really matter, existing code is ok
|
Wow, that's a fast review, thanks. I let Config inherit because it gives a lot of flexibility for free, such as using |
| :param obj: an object | ||
| """ | ||
| for key in dir(obj): | ||
| if key.isupper(): |
There was a problem hiding this comment.
can't you change this for circle with built-in map? As I remember it works faster then for
There was a problem hiding this comment.
By the way, speed should really not be of any concern here, this is only executed at startup. Readability is far more important in this area.
There was a problem hiding this comment.
I just made quick performance test and this really doesn't matter in this case. +1 about readability, so sorry about disturbing :)
|
So, what do you think, @AntonDnepr , leaving as |
|
@Tim-Erwin thank you for explaining. As I said I'm not really experienced in this topic, so I was curious about this and it would be ok as for me. |
Archelyst
force-pushed
the
improved_config
branch
from
c3034b5 to
5926f9b
Compare
Archelyst
force-pushed
the
improved_config
branch
from
5926f9b to
234a792
Compare
|
Ok, I added some documentation. I think I've change everything you asked me to. If there's nothing more to change, I think we're good to go. |
There was a problem hiding this comment.
Gonna wait on one more approval from @channelcat to merge this through and package it into the next release
|
@channelcat , this is marked for release tomorrow. Any thoughts on this? Would love to have this merged. |
|
@Tim-Erwin I'm going to cut a release for 0.2.0 today, I'll move this to the next release. |
|
I'll just continue resolving conflicts :( |
| @@ -15,6 +14,8 @@ | |||
| 'alpha': (str, r'[A-Za-z]+'), | |||
| } | |||
|
|
|||
| ROUTER_CACHE_SIZE = 1024 | |||
There was a problem hiding this comment.
Why not leave this in as a built in configuration value? Then it could be modified by the user in their app with app.config['ROUTER_CACHE_SIZE'] = 2048
There was a problem hiding this comment.
Because it actually cannot be modified. The value is read and applied only once when the sanic module is imported, any further changes don't have any effect.
|
@seemethere , why is that removed from the milestone? This is super easy and backwards compatible and not in a performance critical path and the code is (as mentioned before) borrowed from Flask to quite some extent, so it can be considered tested in production environments. There is documentation and tests and nobody seems to have serious problems with it. If something is missing or not conforming to Sanic standards, I'd be happy to change or add missing parts. Please let me know how to get this done. |
|
Rolling into |
|
Thanks |
Sanic is lacking an easy way to configure an application. This adds basically the a simplified version of Flasks configuration. I added
__getattr__and__setattr__in order to preserve the possibility to access the values with dot-notation.As a side-effect the
ROUTER_CACHE_SIZEgot moved torouter.py. First of all it doesn't really work with the new config class. Second, it was misleading in the first place, because it suggested that it was configurable - but it wasn't (except by changing the code in config.py). When changing the config, thelru_cachedecorator is already fixed and the change has no effect.