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

Allow custom endpoint enumerator #124

Closed
wants to merge 2 commits into from

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Jul 14, 2020

Related to #110

jayvdb added 2 commits July 14, 2020 12:57
Also document custom DEFAULT_GENERATOR_CLASS, and
api_settings.DEFAULT_SCHEMA_CLASS .

Related to tfranzel#110
@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #124 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #124   +/-   ##
=======================================
  Coverage   96.75%   96.75%           
=======================================
  Files          44       44           
  Lines        3022     3023    +1     
=======================================
+ Hits         2924     2925    +1     
  Misses         98       98           
Impacted Files Coverage Δ
drf_spectacular/settings.py 100.00% <ø> (ø)
drf_spectacular/generators.py 95.83% <100.00%> (+0.04%) ⬆️

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 7600cbe...fd182ad. Read the comment docs.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 15, 2020

Note this goes a long way to solving #126 , as this makes it trivial to exclude routes at the beginning.

@jayvdb jayvdb mentioned this pull request Jul 16, 2020
@tfranzel
Copy link
Owner

@jayvdb thanks for the effort. your PR is perfectly fine, but i think i would like to take it in a different direction and making it even easier. i had preprocessing hooks in mind, analogue to postprocessing hooks. even shipping with a default disabled "format stripping" hook that is easily enabled.

i'd prefer to not advertise the existence of the EndpointEnumerator class because they are tricky mechanics and more of an implementation detail. the generator class is just exposed to stay close to upstream. would have stripped that one too. if someone want to go in that deep they prob want to modify the generator with the enumerator and that is easily achieved.

i'll will try test approach and use this PR as a base, but i have to obviously rewrite some of the doc. sry for that.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 17, 2020

Extracted docs out to #128 so we can get it reviewed & merged based on the current situation, and you can build further on top of it if it so pleases you.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 17, 2020

i'd prefer to not advertise the existence of the EndpointEnumerator class because they are tricky mechanics and more of an implementation detail.

That is no different than DEFAULT_GENERATOR_CLASS and even DEFAULT_SCHEMA_CLASS. I can instead implement my own EndpointEnumerator by setting my own DEFAULT_GENERATOR_CLASS which is two lines

class MySchemaGenerator(SpectacularSchemaGenerator):
    endpoint_inspector_cls = MyEndpointEnumerator

I would agree 100% with documenting them all as internal interfaces that are subject to change, and that any breakages will not be included in release notes.

if someone want to go in that deep they prob want to modify the generator with the enumerator and that is easily achieved.

That is obviously not true. This PR shows a very good example of how someone would use the enumerator only to solve problems.

I would guess that most people need to modify the enumerator, and not the schema generator, and frankly the schema generator is a much denser chunk of code for someone to credibly alter, and the schema generator is more brittle, because the AutoSchema is mostly designed now, but there are a lot of bigger picture design problems in the SchemaGenerator, because DRF core never really got to the bigger picture because they hadnt yet solved the details of the structure of the schema. I want to avoid fiddling with the schema generator, but I have lots of requirements that require I alter the endpoints that are ingested.

i had preprocessing hooks in mind, analogue to postprocessing hooks

Great. I went with the simpler approach right now because trying to do a collaborative design in this project would be a waste of time. You present building a preprocessing hook system right now as an alternative to this PR - it isnt. They are orthogonal components, and I would bet my house that your preprocessing hook system either:

  1. exposes an customisable enumerator, or
  2. wont provide a neat way for me to solve my needs that are 'addressed' by this PR.

And if you are going to expose an enumerator, then you are unnecessarily competing with this PR instead of either properly reviewing this PR so we can collaborate on building this small chunk of the puzzle, or merging and rewriting which you like to do.

@tfranzel
Copy link
Owner

i'm sorry that you feel that it is a waste of time. i appreciate your input very much, but in this instance i believe it is the better approach. we should have discussed it more thoroughly in the issue before actually implementing it. in 0852def i provided a possible solution for #93 with an already provided preprocessing hook . feedback is appreciated.

in a way it is what you said in 1, but with a hopefully clearer interface. you can freely modify the operations list while not having to know a great deal about those classes. afais you can address most of your pain points with it. apart from that you are still free to customize the EndpointEnumerator. Sure you have to do 2 extra lines for hooking it in with a custom generator, but guiding the user in the easier direction is worth it i believe.

regarding the Mock request. very good point. this is already done in a similar fashion for versioning support. I think it would be beneficial to do it globally. maybe optionally. yasg is also doing it. we should do a separate issue for that.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 19, 2020

I've got the deeds ready; will check it out now

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 19, 2020

we should have discussed it more thoroughly in the issue before actually implementing it. in 0852def

If you actually meant that, you would have put your code up for review in a PR.

Anyway, as predicted, it is unusable for my needs. I'm off to create my own package which wraps drf-spectacular.

Oh and btw, you broke anyone who had to modify POSTPROCESSING_HOOKS, because they need to re-add postprocess_schema_enums, and then you move it.

ImportError: Could not import 'drf_spectacular.plumbing.postprocess_schema_enums' for API setting 'POSTPROCESSING_HOOKS'. ImportError: Module "drf_spectacular.plumbing" does not define a "postprocess_schema_enums" attribute/class.

@jayvdb jayvdb closed this Jul 19, 2020
@tfranzel
Copy link
Owner

@jayvdb apparently that did send out the wrong signal. i did put it on master but that does not mean we cannot improve or change it still. it is not set in stone. a PR would have been more appropriate i agree.

can you elaborate on why it is unusable for your needs? according to your PR i see 2 main points in code and doc:

  • endpoints modification: preprocessing hook give access to everything the EndpointEnumerator is doing as far as i can see. What functionality are you missing there?
  • mock request: good idea! we should probably provide this by default. i think it would be beneficial.

if you want to further customize the Generator or the AutoSchema, there is still nothing stopping you.

i'm aware of the import change. it will go into the breaking changes section for release. it is the more appropriate place, since plumbing is to be considered internal. trying to keep it clean.

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.

2 participants