-
-
Notifications
You must be signed in to change notification settings - Fork 281
refactor(customize): improve code readability #1412
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
base: master
Are you sure you want to change the base?
refactor(customize): improve code readability #1412
Conversation
316579c
to
39792a0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1412 +/- ##
==========================================
+ Coverage 97.33% 97.59% +0.25%
==========================================
Files 42 57 +15
Lines 2104 2657 +553
==========================================
+ Hits 2048 2593 +545
- Misses 56 64 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
39792a0
to
ce36db5
Compare
I was curious about the difference between https://chatgpt.com/share/68254494-f83c-8010-8ab3-7f8808682bcf probably not important |
info_path = self.custom_settings.get("info_path") | ||
info = self.custom_settings.get("info") | ||
if info_path: | ||
if info_path := self.custom_settings.get("info_path"): | ||
with open(info_path, encoding=self.config.settings["encoding"]) as f: | ||
content = f.read() | ||
return content | ||
elif info: | ||
return info | ||
return "" | ||
return f.read() | ||
return self.custom_settings.get("info") or "" |
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.
Yes, this one was clearly overly complicated 😅
Actually, it matters: if you have
So it really depends on your case, but if there is no default value, and you want to be sure of the type you have in case of falsy value, go for the 2nd. |
ce36db5
to
e163049
Compare
Thank you for explanation. Actually, the thing I'm not sure is if we want to fallback to empty string if the value is falsy (current implementation). |
Description
Make the code shorter but still easy to read.
Checklist
poetry all
locally to ensure this change passes linter check and testExpected behavior
NA, this is refactor.
Steps to Test This Pull Request
Additional context
NA