What about having asgiref and daphne as optional dependencies instead of hard once and raising a proper exception "please install ..." when the import fails?
```
try:
from asgiref import ...
except ImportError:
raise ImportError(
"Please ensure you installed asgiref to use this feature. Do so "
"with `pip install Django[channels]"
)
```
i.e. `pip install "Django[channels]"` ?
/Markus
On Wed, May 04, 2016 at 09:38:33AM -0700, Andrew Godwin wrote:
On Wed, May 4, 2016 at 8:26 AM, Mark Lavin <markdla...@gmail.com> wrote:As noted in the PR there is at least one new setting, CHANNEL_SESSION_ENGINE, which is lacking documentation.That's been added now.The notes on deployment for "Running ASGI under WSGI" don't give any motivation why you would want to do this given that it doesn't support websockets in this case. Is there any advantages? Disadvantages?It lets you still have background tasks and non-WebSocket interface servers (or run websocket interfaces separately). I've expanded the docs to say this.There are tests for the routing and request handling but there are no unittests that I can see for the auth/session handling,Yes, these are missing.Channel/Message/Group/ChannelLayerManager/custom JSON encoder classes and their usage,The ASGI conformance test suite takes care of this: https://github.com/andrewgodwin/django/blob/channels/tests/channels_tests/test_database_layer.py#L3 (that's also used for the inmemory module and asgi_redis in their test suites)or the worker/runworker code.What would you suggest these tests look like? They can't spin up a worker and fire requests at it, and most of the worker code is interactive.The changes to runserver aren't tested.Runserver in Django only has a single test, presumably for the same reason; I don't know how I can write a test for this considering the autoreload and extra process mechanisms we'd want to cover.The new test utilities aren't tested either (yes that's a thing).I would say that the usage of those utilities in other tests means they are tested sufficiently, given that they're used sufficiently; writing a test for just the channel test case would look very similar to the basic tests elsewhere.All new code should have tests. Isn't that Django's current standard?There's no clear standard, the docs just say the tests should "exercise all of the new features". I admit that auth/session needs tests - I'll get those done this week - but I don't see how we can reasonably test the interactive commands in an automated fashion given the current test harness (and lack of tests for things like runserver) in Django.asgiref has one "basic" test around WSGI handling which the usage is documented in this change. I don't think anyone would dare say that covers potential edge cases in WSGI/HTTP handling. Not test related, the status code map is also missing many HTTP status codes.The WSGI handling part of asgiref is not considered complete, and I will probably remove the section about using it in the main Django docs. Only the inmemory backend should be considered used by Django.I'm not in favor of on leeway for external dependencies for the reason you note: they are in the install_requires. Django has avoided having required dependencies and I don't think it sets a good precedent to have the first set be those which don't meet the quality standards expected by the community. That is they don't have sufficient docs/tests to be include in Django itself. It isn't clear why these are required if you are claiming that they aren't required to run. If these we're in the install_requires then I would withdraw the objections about them with regard to this change in Django.With a change I'm going to make, Django will depend on these in install_requires but not actually use them until you try and use a channels feature; they're in install_requires out of user convenience (having a traceback the first time you use the new built-in Django feature seems bad). Considering we're launching Channels in 1.10 as a "provisional" feature, my opinion would be that we can go slightly easier on these dependencies (but of course I'm biased) AndrewOn Wednesday, May 4, 2016 at 10:26:22 AM UTC-4, Andrew Godwin wrote:On Wed, May 4, 2016 at 6:15 AM, Mark Lavin <markd...@gmail.com> wrote:I had assumed this was still a work in progress because there are missing tests and some documentation. The build is passing but the unittest coverage for the new modules seems low or at least not up to the standards I expect for Django contributions. The same for the daphne and asgiref packages which are new requirements. In previous discussions there were talks about performance benchmarks to track potential regressions before this would be accepted similar to the template-based widget rendering change. I don't see any references here or in this work. What is the status of those?- The documentation on Channels is considerably more than I would have accepted for a patch. What do you think is missing? - The channels branch is likely missing some tests around auth and session in particular, but a lot of the "main" stuff has tests; what would you need before accepting it? - asgiref quite literally has more tests than actual code we're using, so I don't see a problem there - Daphne is severely lacking in tests for WebSocket encoding/decoding support as writing a test harness for it is ridiculously hard and I was hoping to get some help on that now we finally have the MOSS money around. - There are no performance regression tests as by default the codepath in Django is the same as before (apart from runserver, which in my basic tests actually sped up); as long as you deploy using WSGI, you never even touch Channels code (which wasn't the original plan, but now is for sanity reasons). I would like some leeway on the external dependencies being fully tested considering that they are not beholden to Django's release cycle, but maybe because we're declaring them as core dependencies (I patched in install_requires into setup.py for them) we should be very strict? If this patch is going to be denied based on Daphne not having sufficient test coverage then I suspect we'll miss the deadline and have to foist this on the LTS instead, which I really don't want. Andrew-- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/0b519047-96ee-4ac9-9230-0f487625a7e2%40googlegroups.com <https://groups.google.com/d/msgid/django-developers/0b519047-96ee-4ac9-9230-0f487625a7e2%40googlegroups.com?utm_medium=email&utm_source=footer> . For more options, visit https://groups.google.com/d/optout.-- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAFwN1uqV3XNhpP45U0wVdGbHv65z1cSECemKAyB8O5dYV0%2B6pA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
-- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/20160504170630.GF9627%40inel.local. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: PGP signature
