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

Why doesn't Protocol use app.config to get its settings #1796

Closed
Tronic opened this issue Feb 27, 2020 · 4 comments
Closed

Why doesn't Protocol use app.config to get its settings #1796

Tronic opened this issue Feb 27, 2020 · 4 comments

Comments

@Tronic
Copy link
Member

Tronic commented Feb 27, 2020

Sanic passes all settings such as REQUEST_MAX_SIZE as arguments via a chain of functions from run_server to Protocol constructor, rather than accessing them via app.config where needed. Is there a purpose to this?

@vltr
Copy link
Member

vltr commented Feb 27, 2020

That's a valid question. IMHO it should respect the config given by the developer (from app.config) and a fallback if not available. Repetition of these configs as parameters (even if grabbed from app.config) should not be necessary. But I believe we would need a broad discussion about that to define.

@Tronic
Copy link
Member Author

Tronic commented Mar 6, 2020

I suppose this could be removed in a future PR if anyone doesn't have objections to that. The APIs affected by this seem to be internal, as app methods such as create_server do not take these arguments.

@ashleysommer
Copy link
Member

ashleysommer commented Mar 16, 2020

@Tronic to answer your question, the reason Protocol doesn't use app.config is simple, the app.config feature didn't exist when the run_server and Protocol constructor code was written in May 2016.

If someone could take it upon themselves to refactor it to use app.config, it would be a good change.

@Tronic
Copy link
Member Author

Tronic commented Mar 16, 2020

That should now be sorted out. It's a large and quickly done patch, though, so I would suggest careful review. I did not remove the loop arguments because that would require more work and has potential to break apps that do weird things with multiple loops.

@Tronic Tronic closed this as completed Mar 16, 2020
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

No branches or pull requests

3 participants