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

If available, use docstrings from properties for field descriptions #954

Merged
merged 3 commits into from
Mar 4, 2023

Conversation

tfranzel
Copy link
Owner

@tfranzel tfranzel commented Mar 4, 2023

supersedes #698

@spookylukey sry this took so long. Thanks for your excellent input and PR. Finally cutting down on the backlog. I had to work this through properly due to the potentially large impact.

I saw a couple of issues with your implementation, but overall liked the idea. My only gripe is that this would potentially leak more (potentially sensitive) info into the schema than expected. I mean this may pull docstrings from objects that are not necessarily related to DRF (e.g. SubObject). Just saying this might come as a surprise to some users, but we advice to check the schema diff on update anyway.

Here are the issues I attempted to fix:

  • For consistency sake we probably should resolve functools.partial as we do in get_type_hints
  • We usually don't use gettattr or cleandoc directly. There are more complications to this and we already solved those in our own get_doc. You probably used a multi-line comment because cleandoc will not clean that up properly 😄. That is a solved issue with get_doc.
  • Not sure if we want to use a docstring when we otherwise had a fail case. seems inconsistent.
  • This also impacts SerializerMethodField methods, which is most likely the more common use-case. Added a test for it.

I'll go ahead and merge this, but if there is anything missing, please don't hesitate to pick this up again. Next round will be fast, I promise 😄

@codecov
Copy link

codecov bot commented Mar 4, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change

Comparison is base (adb6a8e) 98.61% compared to head (17eff93) 98.61%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #954   +/-   ##
=======================================
  Coverage   98.61%   98.61%           
=======================================
  Files          68       68           
  Lines        8164     8183   +19     
=======================================
+ Hits         8051     8070   +19     
  Misses        113      113           
Impacted Files Coverage Δ
tests/test_fields.py 100.00% <ø> (ø)
drf_spectacular/openapi.py 97.43% <100.00%> (+0.01%) ⬆️
tests/test_regressions.py 99.94% <100.00%> (+<0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aqeelat
Copy link

aqeelat commented Mar 10, 2023

@tfranzel Shouldn't this be controlled via a config variable?

@tfranzel
Copy link
Owner Author

tfranzel commented Mar 10, 2023

@aqeelat is there a particular reason you want to turn it off?

We extracted a lot of docstrings before already, for which we didn't have a setting either. This change merely closed a gap/inconsistency.

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.

3 participants