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

Replace if statement with if expression #633

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yezz123
Copy link

@yezz123 yezz123 commented Nov 17, 2021

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:

  • Release (new release request)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (PEP8, lint, formatting, renaming, etc)
  • Refactoring (no functional changes, no API changes)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build related changes (build process, tests runner, etc)
  • Other (please describe):

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #633 (11bf554) into master (eb2010b) will increase coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head 11bf554 differs from pull request most recent head f18da50. Consider uploading reports for the commit f18da50 to get more accurate results
Impacted file tree graph

@@           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           
Flag Coverage Δ
unittests 76.93% <50.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
social_core/strategy.py 84.28% <0.00%> (+2.34%) ⬆️
social_core/storage.py 80.71% <28.57%> (-0.37%) ⬇️
social_core/utils.py 89.26% <100.00%> (-0.36%) ⬇️

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 eb2010b...f18da50. Read the comment docs.

Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good, but I see some more code improvements.

social_core/utils.py Outdated Show resolved Hide resolved
social_core/utils.py Outdated Show resolved Hide resolved
social_core/utils.py Show resolved Hide resolved
social_core/utils.py Outdated Show resolved Hide resolved
@denisSurkov
Copy link

@yezz123 , do you plan to continue work on the pr?

Co-authored-by: Michal Čihař <[email protected]>
@yezz123
Copy link
Author

yezz123 commented Dec 16, 2021

@yezz123 , do you plan to continue work on the pr?

Yes, I was busy just with some work I just merge this f18da50 if you have any improvement in the code that's would be great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants