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

Import app factory before retrieving ioloop fix #156 #157

Closed
wants to merge 1 commit into from

Conversation

vladiibine
Copy link

No description provided.

@vladiibine
Copy link
Author

Mention - I ran the tests, and I don't think I broke anything. One test was broken from before on py3.6.3
- tests/test_start.py:121 test_db_creation[pyloop]

@codecov
Copy link

codecov bot commented Nov 23, 2017

Codecov Report

Merging #157 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
+ Coverage    94.9%   94.91%   +<.01%     
==========================================
  Files          13       13              
  Lines         766      767       +1     
  Branches       93       93              
==========================================
+ Hits          727      728       +1     
  Misses         25       25              
  Partials       14       14
Impacted Files Coverage Δ
aiohttp_devtools/runserver/serve.py 94.32% <100%> (+0.02%) ⬆️

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 cf1cfc9...799489d. Read the comment docs.

@vladiibine vladiibine changed the title Import app factory before retrieving ioloop fix #778 Import app factory before retrieving ioloop fix #156 Nov 23, 2017
@samuelcolvin
Copy link
Member

samuelcolvin commented Nov 23, 2017

Can you provide an example which demonstrates why this is necessary?

My suspicion is that it's not required if your project is set up canonically, however I'm still prepared to make the change if it provides one less headache for users. "There's more than one way to skin a cat".

@@ -109,6 +109,9 @@ def serve_main_app(config: Config, tty_path: Optional[str], loop: asyncio.Abstra
with set_tty(tty_path):
setup_logging(config.verbose)

# imports the factory. This gives users a chance to register alternative event loops
config.app_factory
Copy link
Member

Choose a reason for hiding this comment

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

It's not your fault, but this looks dumb and isn't clear.

If we decide to merge this I think it would be better to remove the app_factory property and rename _import_app_factory > import_app_factory and use that instead.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that making that function public would be a better option. Want me to refactor the function?

Copy link
Author

Choose a reason for hiding this comment

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

Also, regarding my problem, namely being able to set my own event loop, and not use the standard one - details are in the issue #156

There are no other details that I can provide - basically I don't know of a nice way to make the runserver command use the event loop that I want.

If you can suggest a better way, that' fine. I haven't investigated that much, so I don't have an overview of the project's structure.

Until then, the problem remains valid, and both the traceback of the error, and the code that generated it can be found in the issue.

Thanks for your time! :)

@samuelcolvin samuelcolvin self-requested a review November 23, 2017 20:48
@samuelcolvin
Copy link
Member

I thought this had already been solved by #150.

@samuelcolvin
Copy link
Member

Thank you for this, it turns out the fix required a few more changes so I've made #158 to incorporate all the changes required.

I'd be very grateful if you'd review #158.

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.

2 participants