-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat: Convert contentserver to view permanently (drop middleware and flag) #35420
Conversation
…flag) - Delete `content_server.use_view` waffle flag in favor of always using view. - Delete the husk of `StaticContentServerMiddleware` and references to it. - Update views module docstring. This is the first commit of a change/refactor series.
Minimal adjustment of affected import paths, no lint cleanup. This is the second commit in a refactor/change series.
- Remove `wrong-import-order` amnesty comments - Run `isort` on the directory This is the third and last commit in a refactor/change series.
return HttpResponseNotFound() | ||
else: | ||
return response | ||
return IMPL.process_request(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I hate the name IMPL
. I prefer something like STATIC_CONTENT_SERVER_INSTANCE
, or better yet, just replace it with StaticContentServer()
, assuming that instantiation is negligible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no argument there! But I think the next step is to actually just get rid of StaticContentServer
entirely, as it's a class with no state and all the methods are just helper functions that could be dumped into the module, top-level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The only difference is that the former could have been done in 1 minute, and the latter may never actually happen (unless you are planning more clean-up). Oh well. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, it could have been done in 25 minutes, because of the length of time it takes to re-run tests on edx-platform. :-P Sorry, I just really didn't want to do another cycle of that. I do have more cleanup, planned, though -- this was intended as a minimum possible refactor to get rid of the middleware.
…flag) (#35420) Final planned portion of #34702 -- waffle flag and middleware are removed. Commits: 1. Feature change - Delete `content_server.use_view` waffle flag in favor of always using view - Delete the husk of `StaticContentServerMiddleware` and references to it - Update views module docstring 2. Refactor (move) - Move contentserver implementation into views.py 3. Lint cleanup - Fix import ordering (from refactor debris + amnestied lint)
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Final planned portion of #34702 -- waffle flag and middleware are removed.
This is best reviewed commit by commit:
content_server.use_view
waffle flag in favor of always using viewStaticContentServerMiddleware
and references to it