-
Notifications
You must be signed in to change notification settings - Fork 454
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
fix(env_tool): return None if env is empty #1021
Conversation
597b58f
to
28cee90
Compare
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.
Thanks, one nit suggestion:
90081c6
to
c5708f8
Compare
Is anything blocking this? |
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.
Just another suggestion:
Since you have use trim()
before checking .is_empty()
, I'd like the result to also be trim()
.
I was working on something else, but generally I will response within a few hours. Your PR generally looks good to me, just have a little bit to improve on, before merging it in. |
Signed-off-by: DragonBillow <DragonBillow@outlook.com>
c5708f8
to
1e5e295
Compare
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.
Thanks, sorry I somehow didn't get a notification from github
Sometime env maybe a empty value, we should return None in this case: