-
Notifications
You must be signed in to change notification settings - Fork 19.6k
fix: fix data[key] edge case such as an empty string #33779
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: fix data[key] edge case such as an empty string #33779
Conversation
CodSpeed Performance ReportMerging #33779 will not alter performanceComparing
|
5be6358 to
66e5378
Compare
ccurme
left a comment
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.
What is the motivation for this change?
| assert result == "" | ||
|
|
||
|
|
||
| def test_get_from_dict_or_env_none_value() -> None: |
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.
This test passes on master
Hi @ccurme , good question. This is for catching the edge case if the value is 0, False, empty string, etc |
ccurme
left a comment
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.
What is motivating the change?
An existing usage pattern could be to set MY_API_KEY="" to disable picking it up through the env. This would break that usage pattern.
What are we getting in exchange? Is there an open issue?
Hi @ccurme , oh, I see your concern here. If this is the case, for backward compatibility, let me close this PR. Thanks for the thorough consideration! |
(Replace this entire block of text)
Thank you for contributing to LangChain! Follow these steps to mark your pull request as ready for review. If any of these steps are not completed, your PR will not be considered for review.
PR title: Follows the format: {TYPE}({SCOPE}): {DESCRIPTION}
{TYPE}values:{SCOPE}values (optional):PR message: Delete this entire checklist and replace with
Lint and test: Run
make format,make lintandmake testfrom the root of the package(s) you've modified. We will not consider a PR unless these three are passing in CI. See contribution guidelines for more.Additional guidelines:
pyproject.tomlfiles (even optional ones) unless they are required for unit tests. Likewise, please do not update theuv.lockfiles unless you are adding a required dependency.