Cleanup and Dockerfile missing file #57
Conversation
Did a cosmetic cleanup on setup to not have so much empty lines
The Dockerfile was missing config.ini.example file, changed the script to add it
There was a problem hiding this comment.
Hi @michelpereira – thanks for spotting that and opening the PR. I highlighted some changes in inline comments: in general I would accept the PR adding COPY config.ini.example to the task of building the container, but some style changes you made are the main issue I'd like to discuss before merging this. What do you think?
| COPY requirements.txt ./ | ||
| COPY setup ./ | ||
|
|
||
| COPY requirements.txt config.ini.example setup rosie.py ./ |
There was a problem hiding this comment.
I tend to avoid multiple tasks in a single line/command – if something is broken (eg missing file) it's way easier to find out what's wrong.
There was a problem hiding this comment.
Sounds good to me.
| import pip | ||
| from shutil import copyfile | ||
|
|
||
|
|
There was a problem hiding this comment.
Following PEP8 I'm against removing this line. It's a subjective interpretation, but I think the function calls are the main part of this file – ie I'd suggest a two lines separation as PEP8 suggests for top-level function/class definitions.
| @@ -1,9 +1,7 @@ | |||
| #!/usr/bin/env python3 | |||
|
|
|||
There was a problem hiding this comment.
Following PEP8 I'm against removing this line. It's a subjective interpretation, but I think the blank line separates the logic here (shebang from import section).
|
Hey @michelpereira thanks for this PR, would you mind addressing the comments @cuducos made so we can aprove and merge this? I would really like that ;) |
|
I was traveling, will follow up with this pull request. |
|
Do I need to close this pull request and open a new one with the proposed changes? |
No need for that, just commit and push changes to this same branch and the novelties will automagically be in this PR. |
Cleanup in setup and added a missing file to copy on Dockerfile