-
Notifications
You must be signed in to change notification settings - Fork 0
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
Remove drf yasg #24
Remove drf yasg #24
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
==========================================
+ Coverage 54.95% 57.79% +2.83%
==========================================
Files 79 73 -6
Lines 3632 3239 -393
Branches 590 431 -159
==========================================
- Hits 1996 1872 -124
+ Misses 1533 1275 -258
+ Partials 103 92 -11 ☔ View full report in Codecov by Sentry. |
Please don't merge this PR before I review it |
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.
I think there are some drf-yasg leftovers:
- all files in
bin
folder are using drf-yasg now. I thinkpatch_content_types
anduse_external_components
can be removed. So are the related management commands - All files in
inspectors
folder still have drf-yasg imports and code. This should be replaces withdrf_spectacular
extensions - drf-yasg and drf-spectacular have different generation for operation_ids, for the backward compatibility could you keep the same operation_ids as before?
vng_api_common/generators.py
Outdated
|
||
def create_view(self, callback, method, request=None): | ||
if ( | ||
method != "HEAD" |
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.
What happens if method == HEAD
?
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.
This question remains.
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.
Changed
5626773
to
56c2190
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.
I think its fine ¯\_(ツ)_/¯
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.
Whenever Open-Zaak upgrades to this version, I think it can remove some code and reuse code that was added here.
I do think it would be nice to add back the tests for the etag
header code and the custom content-type stuff.
vng_api_common/schema.py
Outdated
AUDIT_TRAIL_ENABLED = apps.is_installed("vng_api_common.audittrails") | ||
|
||
|
||
def _view_supports_audittrail(view: viewsets.ViewSet) -> bool: |
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.
The comment in the permissions files applies here too I guess
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.
This function is just moved from inspectors
folder. I moved it to vng_api_common.audittrails.utils
file
But I agree with you, we can reuse some schema customization from here in Open Zaak, I'll create an issue for it
vng_api_common/schema.py
Outdated
), | ||
] | ||
|
||
def get_override_parameters(self): |
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.
This method is declared twice
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.
fixed
c3bba7a
to
59e682e
Compare
setup.cfg
Outdated
install_requires = | ||
django>=3.2.0 | ||
django-filter>=2.0 | ||
django-solo | ||
djangorestframework>=3.11.0 | ||
djangorestframework_camel_case>=1.2.0 | ||
django-rest-framework-condition | ||
drf-yasg>=1.20.0 | ||
drf-extra-fields>=3.7.0 |
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.
moved it to optional dependencies
vng_api_common/generators.py
Outdated
|
||
def create_view(self, callback, method, request=None): | ||
if ( | ||
method != "HEAD" |
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.
Changed
vng_api_common/generators.py
Outdated
and hasattr(callback.cls, "_conditional_retrieves") | ||
and "head" not in callback.actions | ||
): | ||
# hack to get test_schema_root_tags pass when run in isolation | ||
callback.actions["head"] = callback.cls._conditional_retrieves[0] |
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.
changed
vng_api_common/schema.py
Outdated
), | ||
] | ||
|
||
def get_override_parameters(self): |
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.
fixed
16a7d65
to
7cf4f30
Compare
@SonnyBA Could you please review it? I can't merge it because you request changes |
if method == "HEAD": | ||
return {} | ||
return super().get_overrides(view, method) | ||
# todo support registering and reusing Response components |
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.
Is this meant to be picked up by a separate ticket?
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.
I've created an issue for it - #55
Co-authored-by: SonnyBA <[email protected]>
…ded only for drf-spectaculer field extension
Co-authored-by: SonnyBA <[email protected]>
3c3d9fd
to
9daa877
Compare
Passes test, but please try use it and add test cases where your project breaks.
Fixes #29