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

V6 proposition #156

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

V6 proposition #156

wants to merge 15 commits into from

Conversation

enzofrnt
Copy link
Collaborator

@enzofrnt enzofrnt commented Aug 18, 2024

/!\ DRAFT let me know what you think about

Hi jk, here is another pullrequest form me.
In this PR I have :

  • Reorganise the code (because of the second point)
    put views.py and old viewset.py that now apiview.py in the views folder from which we can now import everything about views and apiview(more latter on the change in the viewset.py which is now apiview.py).
    also move the js file becuase my chnage need a mjor vrsion bump so why not reorganise the code.

  • Transform the viewset to APIView
    I transformed the viewset to an APIView because I believe it is clearer and easier to understand. The Django ViewSet was not the right choice for this use case, so I changed it to an APIView. As a result, it is no longer possible to register the viewset because, as DRF mentions, routers are meant for viewsets and not for APIViews.

    With this change, I also add support for head and option request. To make the code more readable. Also i decide to delete the old methode that use the url path to retrive the channel id. If people would like to use the old methode, they can extend our class and do it by themselves.

    You can also configure the docstring and the name of the class when using configure_events_api_view. This is useful when you want to use the view in the browsable API and you want to have a better description and name of the view.

  • Better renderers config
    With the old redere config thta go through the REST_FRAMEWORK, other APIViews and ViewSets of apps was able to use api_sse and text/event-stream as a renderer. This is not the desired behavior. So by defaut the renderer for our application are define in the code of the APIView and if needed can be overriden in the settings with the EVENTSTREAM_RENDERER variable.

  • Better import for restframework part
    I added in utils a method raise_not_found_module_error which replace all the rest_framework part of the module when the module is not found. This is a better way to handle the error than the previous one.

  • Including tests
    I modify how test are wokring by using only pytest and also provide the vscode config to run test on vscode easly, I don't know if that a great idea. You will decide.
    I also add to the module test cient class which work with sse for restframework and for django also I modified the event.py file to add function to the Event class.
    All older test are still working and I add some new test to test the new feature.
    Current coverage is 59%.

#TODO TEST CI
#TODO IMPROVE THE DOCUMENTATION THAT NOT CLEAR in readme

TODO send_event() from object or data

Proposition :

  • Should we change all the file naming at the same time ? Because currectly thart very hard to read some file name:
    browsableapieventstreamrenderer.py
    browsable_api_event_stream_renderer.py -> maybe way better
    I already name the test file like this to make it more readable.

  • Should we use codecoverage and codecove.io ?
    With it we can display on the repo the current test coverage of the tests and get report of tests.

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.

1 participant