Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants