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.

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

Added 'create' and '--export SHELL' to the cli #270

Closed
wants to merge 0 commits into from

Conversation

jadutter
Copy link
Contributor

@jadutter jadutter commented Aug 5, 2020

  • Added support for specifying when to add 'export' to the dot file
  • Added support for creating dot files from the cli

@coveralls
Copy link

coveralls commented Aug 5, 2020

Coverage Status

Coverage increased (+0.5%) to 90.353% when pulling 5a51bde on jadutter:master into 92ec3b2 on theskumar:master.

@jadutter
Copy link
Contributor Author

jadutter commented Aug 6, 2020

...I'm not sure what else I need to do to help contribute...
I see the coveralls report, but I don't understand how I use this information to increase coverage...

@bbc2
Copy link
Collaborator

bbc2 commented Aug 7, 2020

Thank you for submitting this pull request. I like the idea of being able to make set add the export directive to the .env, since that's a supported feature in the parser, but I think the following additions are not worth it for this project (a bit overkill or too complex):

  • Support for different shells.
  • Validating the file extension.

So what about just adding a bare --export option to set?

About the create command: I'm not sure it's useful since set should implicitly create the file if it doesn't exist, but maybe I'm missing a useful use case?

@jadutter
Copy link
Contributor Author

I tried creating the file through set and it didn't work for me. I only added the create since it seemed weird that I there wasn't a means in the cli to do it. Probably just user error, but I'll double check.

Extension checking only matters if other shells matter, and I was uncertain how many people use it in other shells where export wasn't valid.

I started to add batch (Windows) as a shell option, but the parser failed to read it, and I decided to keep it small and see what other people thought.

@theskumar
Copy link
Owner

Hi @jadutter , thanks for sending the PR!

The set not creating a new file was intentional as I didn't really want any user to be confused where the file is made and the file location was supposed to be conscious decision by the user.

That said, the project has evolved and .env in the root of the folder seem to be the default now and the api's of this project have envolved to treat it like that.

I would prefer to have set command, create a new .env in the current folder if it's not present.

I'm also little concerned around the use-case vs the technical debt this project is going take because of adding export feature for each type of shell.

@jadutter
Copy link
Contributor Author

I'm also little concerned around the use-case vs the technical debt this project is going take because of adding export feature for each type of shell.

I'm not attached to the idea of different shells. I'm just trying to help make it useful to as many people as possible.

Shall I:

  • Remove the multiple shell support
  • Remove the create option
  • Change set to create the file if it does not exist
    And resubmit the PR?

@bbc2
Copy link
Collaborator

bbc2 commented Aug 26, 2020

I think that would be great.

@jadutter
Copy link
Contributor Author

I believe I've made the necessary changes. Please let me know if there's any other changes I can make.

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 overall! Just a few minor comments.

Could you add a simple test case for something simple like set --export a b that checks that the env file then contains export a=b? It might make Coveralls happy but don't worry if it doesn't.

Also, it looks like the CI doesn't run linters and tests anymore (@theskumar any idea why that could be? I don't see Python-dotenv in Travis CI anymore). Could you try running flake8 or tox -e lint locally to check for warnings?

src/dotenv/cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
@jadutter
Copy link
Contributor Author

jadutter commented Sep 3, 2020

Let me know if I should change it so that it does not automatically add the file extension.

@bbc2
Copy link
Collaborator

bbc2 commented Sep 5, 2020

Let me know if I should change it so that it does not automatically add the file extension.

Yes please. I would rather have the user include the extension themselves if they want it.

tests/test_main.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
@bbc2
Copy link
Collaborator

bbc2 commented Sep 6, 2020

I reverted the test, updated the changelog, and squashed the commits. Unfortunately, I pushed the wrong commit to your branch, which closed the PR and prevented me from pushing anything else to it. That's the second time this happens to me; apparently I can't get it right.

Anyway, I created a new PR (#275) with that commit and merged it. Thank you for your contribution! I'll release a new version after I have figured out what's wrong with Travis CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants