-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
BaseSerializer Implemented #13
Conversation
c4ff77a
to
ad65ef6
Compare
df68b70
to
32d4598
Compare
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.
@atb00ker great patch! I appreciate your effort to add the test 👍
Could you address my 2 comments below?
setup.py
Outdated
@@ -33,7 +33,7 @@ | |||
packages=find_packages(exclude=['tests', 'docs']), | |||
include_package_data=True, | |||
zip_safe=False, | |||
install_requires=['django-model-utils'], | |||
install_requires=['django-model-utils', 'djangorestframework'], |
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.
not all the packages using openwisp-utils
have APIs and adding this here would make them download DRF even when not needed.
I suggest the following solution:
- remove this requirement here
- add
djangorestframework>=3.8.2,<3.9
torequirements-test.txt
- add a try/except
ImportError
block when you importserializers
inopenwisp_utils/api/serializers.py
, see my other comment
openwisp_utils/api/serializers.py
Outdated
@@ -0,0 +1,8 @@ | |||
from rest_framework import serializers |
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.
following the text of the other comment, here you could do something like:
try:
from rest_framework import serializers
except ImportError: # pragma: nocover
import sys
print('Django REST Framework is required to use this feature but it is not installed')
sys.exit(1)
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.
How about using django's abstraction for this?
from django.core.exceptions import ImproperlyConfigured
try:
from rest_framework import serializers
except ImportError: # pragma: nocover
raise ImproperlyConfigured('Django REST Framework is required to use this feature but it is not installed')
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.
sure, a lot better! 👍
32d4598
to
f32bc32
Compare
Made the required changes. 😄 |
openwisp_utils/api/serializers.py
Outdated
from rest_framework import serializers | ||
except ImportError: # pragma: nocover | ||
raise ImproperlyConfigured( | ||
'Django REST Framework is required to use this feature but it is not installed') |
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.
we usually break this kind of code into multiple lines with something like:
raise ImproperlyConfigured('Django REST Framework is required to use '
'this feature but it is not installed')
Could you update these 2 lines for consistency reasons please?
f32bc32
to
29f7295
Compare
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.
👍
Added new file
openwisp_utils/api/serializers.py
and implemented a base serializer that can be used in other projects.Fixes #12