Skip to content

Support no value keys - #220

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

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

Conversation

@ulyssessouza

@ulyssessouzaulyssessouza commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

Add support for variables with no value

Closes: #219

@ulyssessouza

ulyssessouza commented Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

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

@bbc2bbc2 left a comment

Copy link
Copy Markdown
Collaborator

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?

Comment thread.travis.yml
Comment threadsrc/dotenv/main.py Outdated
key = to_env(k)
value = to_env(v)
if key and value:
os.environ[key] = value

@bbc2bbc2Jan 15, 2020

Copy link
Copy Markdown
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?

ifkinos.environandnotoverride:
continueifvisnotNone:
os.environ[to_env(k)] =to_env(v)

@ulyssessouzaulyssessouzaJan 15, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

Comment threadsrc/dotenv/main.py Outdated
"""
ret = os.getenv(name, new_values.get(name, ""))
return ret
return ret if ret is not None else name

@bbc2bbc2Jan 15, 2020

Copy link
Copy Markdown
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.

@ulyssessouzaulyssessouzaJan 15, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@bbc2bbc2 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

ulyssessouza commented Jan 15, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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

bbc2 commented Jan 15, 2020

Copy link
Copy Markdown
Collaborator

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

@bbc2bbc2 left a comment

Copy link
Copy Markdown
Collaborator

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:masterJan 15, 2020
@ulyssessouza

ulyssessouza commented Jan 16, 2020

Copy link
Copy Markdown
ContributorAuthor

@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 😺)

@ulyssessouzaulyssessouza mentioned this pull request Jan 16, 2020
Merged
@ulyssessouza

ulyssessouza commented Jan 16, 2020

Copy link
Copy Markdown
ContributorAuthor

Draft PR on docker-compose

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

Reviewers

@bbc2bbc2bbc2 approved these changes
@theskumartheskumarAwaiting requested review from theskumar

Assignees

@bbc2bbc2

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