Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support no value keys #220

Merged
merged 2 commits into from
Jan 15, 2020
Merged

Support no value keys #220

merged 2 commits into from
Jan 15, 2020

Conversation

ulyssessouza
Copy link
Contributor

Add support for variables with no value

Closes: #219

@ulyssessouza
Copy link
Contributor Author

Please note that I removed Python 3.4 on .travis to avoid incompatibility with PyYAML since it has reached it's EOL since mid 2019

bbc2
bbc2 requested changes Jan 15, 2020
View reviewed changes
Copy link
Collaborator

@bbc2 bbc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this work.

I think it's OK to accept variables without a value but I don't agree with all the changes made in main.py. However, the changes in parser.py look fine.

I'm curious: What functions or classes of this project would Docker-compose use?

.travis.yml Show resolved Hide resolved
src/dotenv/main.py Outdated
key = to_env(k)
value = to_env(v)
if key and value:
os.environ[key] = value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite complicated. What about simplifying this to the following?

            if k in os.environ and not override:
                continue
            if v is not None:
                os.environ[to_env(k)] = to_env(v)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!
That was me juggling with mypy errors and trying to test everything with None 😄

PTAL

src/dotenv/main.py Outdated
@@ -206,7 +209,7 @@ def _replacement(name):
then look into the dotenv variables
"""
ret = os.getenv(name, new_values.get(name, ""))
return ret
return ret if ret is not None else name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ret can't be None here, so I think the if is not None test shouldn't be added. Also, returning name looks incorrect because "" should be the default value.

It seems that Mypy detects an unrelated issue, but in that case I would suggest adding # type: ignore, if necessary, since it's probably a false positive from Mypy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same mypy juggling here. I'll just ignore
PTAL

@bbc2 bbc2 self-assigned this Jan 15, 2020
@ulyssessouza
Support no value keys
6f72b6e 
Signed-off-by: ulyssessouza <ulyssessouza@gmail.com>
@ulyssessouza
Remove Python 3.4 in .travis (EOL mid 2019)
30b0b0f 
Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
@ulyssessouza
Copy link
Contributor Author

ulyssessouza commented Jan 15, 2020 •
edited

@bbc2

Thank you for this work.

You are welcome!

I'm curious: What functions or classes of this project would Docker-compose use?

The idea is to use dotenv_values. That works way better than our home brewed version. We had some bugs created relative to it's non-standard way to treat .env files.

@bbc2
Copy link
Collaborator

bbc2 commented Jan 15, 2020

The idea is to use dotenv_values. That works way better than our home brewed version. We had some bugs created relative to it's non-standard way to treat .env files.

OK good to know. dotenv_values should work well. Parsing .env files is tough indeed, notably because there's no commonly-used specification for them.

bbc2
bbc2 approved these changes Jan 15, 2020
View reviewed changes
Copy link
Collaborator

@bbc2 bbc2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. Nice rebasing. 👍

@bbc2 bbc2 merged commit c00ccbb into theskumar:master Jan 15, 2020
@ulyssessouza
Copy link
Contributor Author

@bbc2 Any expectations on a release?

For now I'm vendoring from my branch, but I cannot merge into docker-compose's master this way. I would need an official release for that.
(No pressure, just saying 😺)

@ulyssessouza ulyssessouza mentioned this pull request Jan 16, 2020
Merged
@ulyssessouza
Copy link
Contributor Author

Draft PR on docker-compose

@bbc2 bbc2 mentioned this pull request Jan 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbc2 bbc2 bbc2 approved these changes

@theskumar theskumar Awaiting requested review from theskumar

Assignees

@bbc2 bbc2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for variables with no value
2 participants
@ulyssessouza @bbc2