Closed vitorkusiaki closed 7 years ago
Hey @vitorkusiaki thanks for this ;)
This is getting the same error as #132 waiting the restarted build to take a closer look into it. But as @cuducos said it doesn't seem related to the PR itself.
@jtemporal @cuducos hello! It seems the build has passed and the error was not related to this PR. Is it ok now? Any improvement for me to do?
@vitorkusiaki, first some pending reviews need to be addressed on #132 before merge this one.
Hi! @vitorkusiaki liked this a lot! Thank you for this PR! As @lipemorais said earlier, I left a code review and am waiting for the changes on #132 😉
Once they are done we'll need to update your code, but I believe everything is okay here.
I tested this in two stages:
Running the tests to see if everything is passing
git checkout -b vitorkusiaki-dataset-all-update master
git pull https://github.com/vitorkusiaki/serenata-toolbox.git dataset-all-update
python -m unittest discover tests/unit
Since this PR will mostly benefit other repositories setups I used serenata-de-amor
environment to test this:
requirements.txt
in serenata-de-amor
repository so it would install the toolbox from @vitorkusiaki fork by replacing serenata-toolbox
with git+https://github.com/vitorkusiaki/serenata-toolbox.git@dataset-all-update
serenata_de_amor
environment activated, ran the setup
which gave me the following error:
requirements.txt
to avoid overwriting the previous "hack"setup
again and everything workshey @vitorkusiaki good news! #132 was merged, why don't you go ahead and fix the conflicts here so we can merge this?
This will be awesome and will also free datasciencebr/serenata-de-amor#273 to be merged!
Let me know if I can do anything to help out 😉
Hello @jtemporal @lipemorais @cuducos! Sorry it took so long, but I managed to update this PR.
Take a look at the last commit and tell me what you think. The travis build is still failling, but the test that failed is from another file that it's not on this PR scope. Looking forward for your feedback!
Thanks
Hey, @vitorkusiaki! There are some conflicts here with master branch. Could you fix this so we will be able to merge.
It was a version conflict. It is fixed, @lipemorais. Thanks, hadn't noticed.
This PR contains commits from #132, so please merge #132 first.
If the user wants to download all the datasets, even if they are already downloaded, he can pass an option
--all
to the command./setup
. Otherwise, only the missing datasets will be downloaded.