Fix #1082 by ignoring proxy parameter if env parameter is set but empty#1083
Fix #1082 by ignoring proxy parameter if env parameter is set but empty#1083seratch merged 3 commits intoslackapi:mainfrom
Conversation
|
|
| ) | ||
| if proxy_url is None or len(proxy_url.strip()) == 0: | ||
| # If the value is an empty string, the intention should be unsetting it | ||
| if len(proxy_url.strip()) == 0: |
There was a problem hiding this comment.
This line does not work if the proxy_url is None. Can you verify it with a new test pattern in tests/test_proxy_env_variable_loader.py?
>>> proxy_url = None
>>> len(proxy_url.strip())
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'NoneType' object has no attribute 'strip'How about doing this way?
if proxy_url is None:
return None
if len(proxy_url.strip()) == 0:
# If the value is an empty string, the intention should be unsetting it
logger.debug("...")
return None| if proxy_url is None or len(proxy_url.strip()) == 0: | ||
| # If the value is an empty string, the intention should be unsetting it | ||
| if len(proxy_url.strip()) == 0: | ||
| logger.debug("HTTP proxy env variable is set but empty") |
There was a problem hiding this comment.
Perhaps, telling what happened would be more informative:
| logger.debug("HTTP proxy env variable is set but empty") | |
| logger.debug("The Slack SDK ignored the proxy env variable as an empty value is set.") |
| url = load_http_proxy_from_env() | ||
| self.assertEqual(url, "http://localhost:9999") | ||
|
|
||
| def test_load_all_empty_case(self): |
There was a problem hiding this comment.
Can you add one more test with None values?
|
@y-adachi-00one Would you mind signing our CLA? Without having it, we are unable to merge your contribution to this project. See the above bot comment for details. |
|
Hi @y-adachi-00one, thanks for signing the CLA! No rush at all but could you update this pull request if you have a chance? |
|
I'm sorry for late. I'll modify that according to your advice. Could you give me a little time? |
|
No rush at all. We're going to release v3.9 very soon but your changes can be included later on (v3.10 or later)! |
|
@seratch Thank you for reviewing. I fixed my code according to your advice. |
seratch
left a comment
There was a problem hiding this comment.
Looks great to me! Once the CI builds pass, we will merge this PR 👍
Codecov Report
@@ Coverage Diff @@
## main #1083 +/- ##
==========================================
- Coverage 86.07% 86.06% -0.02%
==========================================
Files 110 110
Lines 9932 9936 +4
==========================================
+ Hits 8549 8551 +2
- Misses 1383 1385 +2
Continue to review full report at Codecov.
|
Summary
Fix #1082
Category (place an
xin each of the[ ])/docs-src(Documents, have you run./docs.sh?)/docs-src-v2(Documents, have you run./docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.