Replace if statement with if expression #633
Conversation
Codecov Report
@@ Coverage Diff @@
## master #633 +/- ##
=======================================
Coverage 76.92% 76.93%
=======================================
Files 317 317
Lines 9639 9630 -9
Branches 469 465 -4
=======================================
- Hits 7415 7409 -6
+ Misses 2073 2070 -3
Partials 151 151
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@yezz123 , do you plan to continue work on the pr? |
Co-authored-by: Michal Čihař <[email protected]>
|
@yezz123 is there more work to do on this PR beside the conflicts to be resolved ? |
|
@gersona I think another review that it doesn't change the behavior would be a good start. |
| if hasattr(user, 'is_active'): | ||
| return user.is_active() if callable(user.is_active) else user.is_active | ||
| else: | ||
| return True |
There was a problem hiding this comment.
Same, it should explicitly return False here; some tests expect a boolean, not just a truthy/falsy value, as in social_core.tests.test_utils.UserIsActiveTest
| else: | ||
| authenticated = False | ||
| return authenticated | ||
| return True | ||
|
|
There was a problem hiding this comment.
I should explicitely return a boolean as some tests expect it to be so, e.g social_core.tests.test_utils.UserIsAuthenticatedTest
There case were user is truthy but has no is_authenticated attribute is not covered here.
The fallback value could be something like
return bool(user)
# or
return True if user else False
| else: | ||
| authenticated = False | ||
| return authenticated | ||
| return True |
There was a problem hiding this comment.
This no longer checks whether user is authenticated.
Proposed changes
Python's conditional expression syntax is its version of the ternary operator. Using this syntax is definitely more concise, but it is one of the more controversial refactorings (along with list comprehensions). Some coders dislike these expressions and find them slightly harder to parse than writing them out fully.
Types of changes
Please check the type of change your PR introduces:
Checklist