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

Config from object string #1436

Merged
merged 13 commits into from
Jun 16, 2019

Conversation

jotagesales
Copy link
Contributor

Implemented support in config_from_object method to receive a path by sting equal similar to Flask.

@codecov
Copy link

codecov bot commented Dec 26, 2018

Codecov Report

Merging #1436 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1436      +/-   ##
==========================================
+ Coverage   91.35%   91.41%   +0.05%     
==========================================
  Files          18       18              
  Lines        1781     1793      +12     
  Branches      337      339       +2     
==========================================
+ Hits         1627     1639      +12     
  Misses        130      130              
  Partials       24       24
Impacted Files Coverage Δ
sanic/config.py 100% <100%> (ø) ⬆️
sanic/helpers.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d581315...b534df2. Read the comment docs.

setup.py Outdated Show resolved Hide resolved
@vltr
Copy link
Member

vltr commented Dec 27, 2018

Also, take a look at #1424 (as reference) to provide documentation to the import_string function and for the official docs 😉

Copy link
Contributor

@harshanarayana harshanarayana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to add a full-length example into the config.md file explicitly for this behavior.

docs/sanic/config.md Show resolved Hide resolved
module_name is a valid path to class

"""
module, klass = module_name.rsplit(".", 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be overthinking this, but would it be a good idea to support relative string paths?

Copy link
Contributor Author

@jotagesales jotagesales Dec 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, i use this function in Flask for configure app using config_from_object and is very good.
Because is common in application has config classes.
example:

class Config:
      DEBUG = False

class Develop(Config):
    DEBUG = True

class Production(Config):
      pass

In this case the user instanciate Sanic class and configure according from environment pass the string path of the class get from environment variable for example.

Copy link
Contributor

@harshanarayana harshanarayana Dec 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the user instanciate Sanic class and configure according from environment pass the string path of the class get from environment variable for example.

Oh I am totally with you on this feature. I was just curious if it might be possible to support relative path as well.

i.e. Instead of module1.module2.Class, if you are already inside module1, then module2.Class should suffice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harshanarayana I think that the importlib searches the entire loaded packages, so I guess this already works out of the box - I might be wrong, testing still needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vltr It works, but the method invocation might need to be tweaked a bit to provide another helper argument. import_module(name, package=None)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harshanarayana oh, ok, got it. Some tweaking may be needed then ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jotagesales looks like this is still outstanding - can you make changes so we can get this in soon?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjsadowski ready ;)

class Config:
not_for_config = "should not be used"
CONFIG_VALUE = "should be used"
class Config:
Copy link
Contributor

@harshanarayana harshanarayana Mar 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rename this class from Config to something else? This line is overriding the import done by from sanic.config import Config

This is causing tests to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harshanarayana i renamed the class, thank.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming this class seems to be the only thing holding it up.

@ahopkins ahopkins dismissed their stale review April 30, 2019 16:56

Dependency was removed from setup.py

@yunstanford yunstanford merged commit 8fbbe94 into sanic-org:master Jun 16, 2019
@yunstanford yunstanford added this to the 19.6 milestone Jun 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants