-
-
Notifications
You must be signed in to change notification settings - Fork 9
Update README.md #14
base: master
Are you sure you want to change the base?
Update README.md #14
Conversation
updating README.md with more explicit initialization steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements, added couple of comments, but looks good to me.
README.md
Outdated
|
||
- Create a conda enviroment based on environment.yml | ||
```sh | ||
conda env create -f environment.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the -f environment.yml
if the name of the file is environment.yml
, since it's conda's default. Feel free to leave it if you prefer to be explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know that was the default, good to know.
README.md
Outdated
- https://github.com/MacPython/pandas-wheels | ||
- https://github.com/pandas-dev/pandas | ||
|
||
- Be sure to initialize repos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thing, but may be a colon at the end to show that you're talking about the next command (thought initially you were talking about the previous list of repos)
Implemented comments made on last PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for the update
|
||
- Create a conda enviroment based on environment.yml: | ||
```sh | ||
conda env create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be conda env create -f environment.yaml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
environment.yml
is the default conda file name, so not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, even if it is the default, I find leaving it out just confusing (for all people that don't know or directly remember that this is the default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I see the non-explicit form is confusing to people. will change it back :D
- https://github.com/MacPython/pandas-wheels | ||
- https://github.com/pandas-dev/pandas | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required for anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It introduces a newline. for better readability and clearer separation between the commands
Doesn't need to be part of this PR, but in running for the first time I found "make sure docker daemon is installed+running" would be useful |
updating README.md with more explicit initialization steps