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

add pipenv file #5

Merged
merged 4 commits into from
Oct 14, 2019
Merged

add pipenv file #5

merged 4 commits into from
Oct 14, 2019

Conversation

Trafitto
Copy link
Contributor

@Trafitto Trafitto commented Oct 8, 2019

I hope it goes well, I had never used pipenv #5

@Trafitto Trafitto mentioned this pull request Oct 8, 2019
Copy link
Contributor

@treuille treuille left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request @Trafitto ! Very cool!

Before this can be approved can you please:

  1. Resolve the code comment changes.
  2. Indicate how you tested the correctness of this pipfile (for example by creating a clean virtual environment and using it to install dependencies)
  3. Wait for a code review by @monchier.

Note to @monchier : Do you think we even need this pipfile? Please be honest!

Pipfile Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
Pipfile Show resolved Hide resolved
Pipfile.lock Outdated Show resolved Hide resolved
Copy link
Collaborator

@monchier monchier left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Trafitto! Please address the comments.

Pipfile Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@Trafitto
Copy link
Contributor Author

Ok, I make the requested changes

Copy link
Contributor

@treuille treuille left a comment

Choose a reason for hiding this comment

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

Can you please:

  1. Make @monchier's changes.
  2. Remove the Pipfile.lock

Then I will approve the PR. Thanks, @Trafitto ! ❤️

@Trafitto Trafitto requested a review from a team as a code owner October 12, 2019 09:33
@Trafitto
Copy link
Contributor Author

I made the requested changes.

Here are the steps to create pipenv:
pipenv shell
pipenv install streamlit
pipenv opencv-python
Use pipenv check for checking the pipfile
Use pipenv install for installing all dependencies

Copy link
Collaborator

@monchier monchier left a comment

Choose a reason for hiding this comment

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

A couple more edits.

.gitignore Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
[dev-packages]

[packages]
streamlit = ">0.47.4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is not 0.47.0 enough for this to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the version suggested by @treuille

Streamlit version must be at least 0.47.4

Copy link
Collaborator

@monchier monchier left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution @Trafitto !!! Looks good to me.

Copy link
Contributor

@treuille treuille left a comment

Choose a reason for hiding this comment

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

LGTM

@treuille treuille merged commit cecd09b into streamlit:master Oct 14, 2019
@treuille
Copy link
Contributor

Congratulations on your first merge to the repo @Trafitto!

@Trafitto
Copy link
Contributor Author

Thanks 👍

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.

3 participants