-
-
Notifications
You must be signed in to change notification settings - Fork 48
Support for non-default dataclass fields #116
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
Conversation
This is small change, any reason why this is not merged? |
This looks all fine, but needs a rebase and some minor renames/etc. I'll go ahead and do it, and see how CI reacts. |
c83e14f
to
91f6fc5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
==========================================
- Coverage 89.83% 89.38% -0.45%
==========================================
Files 27 27
Lines 1141 1168 +27
==========================================
+ Hits 1025 1044 +19
- Misses 116 124 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Some minor comments, and that this will need a changelog entry. Given the age of the PR I may just go ahead and do the changes to ensure this gets in.
if dataclasses.is_dataclass(obj): | ||
if attr in obj.__dataclass_fields__: | ||
return obj.__dataclass_fields__[attr].name | ||
else: | ||
# raise original AttributeError | ||
raise e | ||
else: | ||
# raise original AttributeError | ||
raise e |
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 logic here could be simplified a bit and all put into one conditional and a raise outside of it.
return getattr(obj, attr, *defargs) | ||
try: | ||
return getattr(obj, attr, *defargs) | ||
except AttributeError as e: |
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.
extreme nitpick: it's best to avoid single-letter variables and use something descriptive, even when the single letter feels trivial.
This PR looked stale. I addressed the comments and made some other minor improvements in #208. |
Superseded by #208 |
Support for non-default dataclass fields (continuation of stale PR, #116)
Hi, this PR tries to fix this issue: #115.
Used sphinx version: 3.2.1
Python version: 3.8.5
Fix #115