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

Middleware support #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

inforian
Copy link

@inforian inforian commented Sep 4, 2017

  • Instead of importing Mixin in every View, Do this job using middleware is more durable.

@inforian
Copy link
Author

inforian commented Sep 4, 2017

  • Sorry I was crating the pull request on my fork but instead created on Parent repo. So you can ignore This pull request.

  • However if you liked the idea , I can explain what I did and why ? and can write test cases too

@avelis
Copy link
Collaborator

avelis commented Sep 8, 2017

@inforian Thanks for making a PR. If you can briefly explain how this betters the library I am all for it. Write somes tests would be great. Lastly, add some quick documentation on how it should be installed.
Would this be a breaking change for current users? Or would does it compliment the mixin?

@inforian
Copy link
Author

inforian commented Sep 8, 2017

  • The main idea is to avoid redundancy of using Mixin in every viewset. Instead their must be a way where you have to mention just once and that will be applicable everywhere (Some configurable settings can be provided).

  • So I decided to write a Django middleware for the same which will use this mixin functions like :

    • In process_request -> initial
    • process_exception -> handle_exception
    • process_response -> finalize_response
  • But during Implementation I faced some issues like :
    - Your Mixin use DRF Request Object but Middleware will pass HttpRequest object , So some things may not be accessible in Mixin like request.data .

    • In finalize_response some response do not have rendered_content
    • calling super_class super(BaseLoggingMixin, self).any_function(request, *args, **kwargs)
  • However I fixed above issues but then I realize This repo aims at DRF not Django , So a Middlware won't be a good option. But I still think there must be a single way of enabling this logging functionality instead of use Mixin in every Viewset.

  • So I wanted to contribute and enable above functionality (keeping the old one too) but first I wanted to know your views and Any thoughts on what will be the best way to do so ?

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.

3 participants