Skip to content

Support no value keys#220

Merged
bbc2 merged 2 commits intotheskumar:masterfrom
ulyssessouza:add-no-value-keys
Jan 15, 2020
Merged

Support no value keys#220
bbc2 merged 2 commits intotheskumar:masterfrom
ulyssessouza:add-no-value-keys

Conversation

@ulyssessouza
Copy link
Contributor

@ulyssessouza ulyssessouza commented Jan 14, 2020

Add support for variables with no value

Closes: #219

@ulyssessouza
Copy link
Contributor Author

ulyssessouza commented Jan 14, 2020

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?

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

@bbc2 bbc2 Jan 15, 2020

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

@ulyssessouza ulyssessouza Jan 15, 2020 •
edited
Loading

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
"""
ret = os.getenv(name, new_values.get(name, ""))
return ret
return ret if ret is not None else name
Copy link
Collaborator

@bbc2 bbc2 Jan 15, 2020

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

@ulyssessouza ulyssessouza Jan 15, 2020 •
edited
Loading

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 ulyssessouza requested a review from bbc2 January 15, 2020 20:17
@ulyssessouza
Copy link
Contributor Author

ulyssessouza commented Jan 15, 2020 •
edited
Loading

@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

ulyssessouza commented Jan 16, 2020

@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

ulyssessouza commented Jan 16, 2020

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