-
Notifications
You must be signed in to change notification settings - Fork 89
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
enzofrnt
wants to merge
15
commits into
fanout:master
Choose a base branch
from
enzofrnt:V6-Proposition
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
V6 proposition #156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drf importations to make IDE completion work better
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
/!\ 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 oldviewset.py
that nowapiview.py
in theviews
folder from which we can now import everything about views and apiview(more latter on the change in theviewset.py
which is nowapiview.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
andtext/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 theAPIView
and if needed can be overriden in the settings with theEVENTSTREAM_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.