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

app.config.from_object() missleading name -> app.config.from_module() would be better ? #1895

Closed
tomaszdrozdz opened this issue Jul 10, 2020 · 12 comments

Comments

@tomaszdrozdz
Copy link
Contributor

tomaszdrozdz commented Jul 10, 2020

Name of method
app.config.from_object()
is missleading somehow.

I thought I could:

my_conf = {'MY_SETTING': 1}
app.config.from_object(my_conf)

but it does not work.

In such case I simply have to:

app.config.update(my_conf)

app.config.from_object(my_conf)
only works in such scenario:

# my_conf.py
MY_SETTING = 1

# my_app.py
import my_conf
app.config.from_object(my_conf)

So I suggest maybe the name of this method should be
app.config.from_module(...) ???

@tomaszdrozdz
Copy link
Contributor Author

tomaszdrozdz commented Jul 10, 2020

Or if follow documentation:

You could use a class or any other object as well.

then we have bug.

So mayby it shoud be fixed ?

@ahopkins
Copy link
Member

That method works on more than just modules. It would also work on classes or instances. It is more of a duck typing than anything else, as it looks for properties on an object.

@tomaszdrozdz
Copy link
Contributor Author

tomaszdrozdz commented Jul 10, 2020

# sanic/config.py
# put print(key) into 

def from_object(self, obj):
    if isinstance(obj, str):
        obj = import_string(obj)
        for key in dir(obj):
            print(key)                 # add print here
            if key.isupper():
                self[key] = getattr(obj, key)
c = {'TEST': 111111111111111111111111111111111111}
app.config.from_object(c)

Start server and observe what is going on:
it will go with dir(c)
but I would expect it to go with c.keys()

And if You later check app.config You will not see there is TEST.


for key in dir(obj):

only works for module.

For object we shoud have:

for key in obj:

or

for key in obj.keys():

So again:
either we have bug
or the method should be named accordingly.

@ahopkins
Copy link
Member

I agree, the usage of dir there is a bizarre choice. Not what I would use for a class, an instance or a module.

@tomaszdrozdz
Copy link
Contributor Author

How to deal with it ?

@tomaszdrozdz
Copy link
Contributor Author

We have:

  • modules for which we could use some_module.__dict__.keys()
  • dictionaries for which we could use some_dict.keys()
  • objects for which we could use some_object.__dict__.keys() but only for "self" members, but for "class" members we should use some_object.__class__.__dict__
  • etc

I am afraid we can not deal with all this cases with one simple method "from_object".

So maybe it would be better to resign from "from_object()",

and maybe create more fine tuned methods like "from_module()", "from_instance()", ...

or maybe just one simple method "from_dict()" that would filter ony upper case keys
and put in documentation how to use it like:

  • app.config.from_dict(some_dict)
  • app.config.from_dict(some_module.__dict__)
  • ...

@ahopkins
Copy link
Member

I think I like the more intuitive naming.

app.config.from_module
app.config.from_instance
app.config.from_dict

Then, I would leave from_object as is, untouched, except for a warning:

 warnings.warn('Usage of from_object() is being deprecated, and will be removed in v21.3. Please user from_module(), from_instance(), or from_dict().')

Thoughts?

@tomaszdrozdz
Copy link
Contributor Author

tomaszdrozdz commented Jul 31, 2020

Although,
second thoughts:

from_module(), from_instance() are not different,
and instance can also be a class.

I guest from_object() was introduced because we want to keep Sanic
as "minimum" as possible from one side,
but as elastic / feature full as possible (within this minimality) from another side.

So to keep it that way maybe:

Aproach 1)

from_dict(x)  
# as name suggest  

from_object(x)  
# that would internaly call  
    from_dict(x.__dict__)  
# so we could pass here any object having __dict__, like: instance, class, module  

# Also I guess should have then:  
from_file("path/to/py/file")  

Aproach 2)

# Modify  
from_object(x)  
# so it can detect if it deals with dictionary, and acts accordingly,  
# or "string" so it is path fo file and acts acordingly  
# or in other case take x.__dict__ 

And leave:

from_envvar() 
# as is, perhaps change its name to from_file_in_envvar()

@ahopkins
Copy link
Member

I'm not keen on changing the existing behavior. If we want a more proper method to load from any sort of input, then maybe it's app.config.load.

Between now and the end of the year we are on a breaking change freeze.

@ahopkins
Copy link
Member

You are 100% right though. Unopinionated and flexible, but smart defaults and easy to use api to enable development.

@tomaszdrozdz
Copy link
Contributor Author

Can You please take a look at this (#1903) aproach ?

I was able to replace:

from_envvar()  
from_object()  
from_pyfile()  

with one smiple (6 lines of code):

update_config()  

and replace "loading module from file code" in

from_pyfile()  

in a favour of

load_module_from_file_location()  

that uses python 3.5+ method to load module from file path
but also deals with environment variables provided in that "path".

@tomaszdrozdz
Copy link
Contributor Author

We refactored it in #1903
so I can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants