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

Adds a bit about redoc logo in docs, adds a custom_path_ordering hook, adds ability for configuring hooks from config #410

Closed
wants to merge 7 commits into from

Conversation

allen-munsch
Copy link

Adds a bit about redoc logo in docs, adds a custom_path_ordering hook, adds ability for configuring hooks from config

@codecov
Copy link

codecov bot commented May 31, 2021

Codecov Report

Merging #410 (c238aef) into master (2684fda) will decrease coverage by 0.28%.
The diff coverage is 36.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
- Coverage   98.51%   98.22%   -0.29%     
==========================================
  Files          55       55              
  Lines        5394     5416      +22     
==========================================
+ Hits         5314     5320       +6     
- Misses         80       96      +16     
Impacted Files Coverage Δ
drf_spectacular/settings.py 100.00% <ø> (ø)
drf_spectacular/hooks.py 84.76% <11.11%> (-15.24%) ⬇️
drf_spectacular/generators.py 97.93% <100.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2684fda...c238aef. Read the comment docs.

Copy link
Owner

@tfranzel tfranzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @allen-munsch, thanks for the PR. i think i need a bit more context. please correct me if i'm wrong but i believe we already have this functionality with PREPROCESSING_HOOKS. see comments below.

furthermore it would break everyone using POSTPROCESSING_HOOKS. we really need a very good reason to even consider breaking compatibility.

docs/settings.rst Show resolved Hide resolved
@@ -10,6 +10,38 @@
from drf_spectacular.settings import spectacular_settings


def custom_path_ordering(result, generator, **kwargs):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the need for custom endpoint order.

to me that looks like a perfect usage of PREPROCESSING_HOOKS: https://drf-spectacular.readthedocs.io/en/latest/customization.html#step-7-preprocessing-hooks

there you can filter and reorder the endpoints to your liking. Not sure if this adds anything substantial to that existing functionality?

@allen-munsch
Copy link
Author

allen-munsch commented May 31, 2021

@tfranzel yeah i dunno preproccess hook didn't seems to keep the ordering

also the core hooks are not configurable, sure can keep configs bundled with the hooks, but i dunno settings allover the place instead of all in the settings.py seems werid.

and i didnt see offhand how restframework APISettings would allow that alongside the dot imports strings

@tfranzel
Copy link
Owner

@tfranzel yeah i dunno preproccess hook didn't seems to keep the ordering

that is an excellent point. i noticed there is sorting applied after the PREPROCESSING_HOOK. we should make that final sorting optional via settings. the sorter was once configurable but was superceded by the hooks. would that work for you?

also the core hooks are not configurable, sure can keep configs bundled with the hooks, but i dunno settings allover the place instead of all in the settings.py seems werid.

there should be nothing preventing you from loading the django/DRF settings inside the hook and then using them. then you have everything in your settings.py. am i missing something?

@tfranzel
Copy link
Owner

come to think of it. we have SORT_OPERATION_PARAMETERS with True/False/callable. would seem logical to do the same here. then you could choose between doing it in a hook or writing a sorting callable yourself.

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.

None yet

2 participants