Improve get_apikey method and its tests #19
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19 +/- ##
==========================================
- Coverage 59.82% 59.55% -0.27%
==========================================
Files 14 14
Lines 448 450 +2
Branches 57 58 +1
==========================================
Hits 268 268
- Misses 179 181 +2
Partials 1 1
|
| try: | ||
| return os.environ[ENV_VARIABLE] | ||
| except KeyError: | ||
| if not key: |
There was a problem hiding this comment.
This is a behavior change - it was intentional that empty string is different from not passing a key. Do you have this issue because of Scrapy integration, where it is hard to set a setting to None from the command line?
There was a problem hiding this comment.
it was intentional that empty string is different from not passing a key
What's the desired outcome with this behavior?
Do you have this issue because of Scrapy integration, where it is hard to set a setting to None from the command line?
Actually, no. Tests have failed on my machine because I had this variable set and I thought about "fixing" this blank string problem while creating a fixture that patched system environment during tests.
I don't understand why it would be desired to have a blank string returned from the get_apikey() method since it's an invalid value and will culminate in a failed request. But if you'd like me to I can just revert the logic and keep focused on the test fixture to avoid failing because of my own environment.
There was a problem hiding this comment.
Thanks for the explanation @victor-torres! I'm fine with this change, but we need to make sure we mention it in the changelog as technically backwards incompatible.
| @@ -9,6 +9,6 @@ def test_get_apikey(autoextract_env_variable): | |||
| assert get_apikey() == autoextract_env_variable | |||
|
|
|||
|
|
|||
| def test_get_apikey_missing(): | |||
| def test_get_apikey_missing(autoextract_blank_env_variable): | |||
There was a problem hiding this comment.
hm, technically this is a different test case, and previous case (an absent env varibale) is not covered now
This pull request: