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 option to update all the datasets #133

Merged
merged 3 commits into from
Oct 3, 2017

Conversation

vitorkusiaki
Copy link
Contributor

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.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 48.419% when pulling 72d9319 on vitorkusiaki:dataset-all-update into d53d3f3 on datasciencebr:master.

@coveralls
Copy link

coveralls commented Aug 19, 2017

Coverage Status

Coverage increased (+37.3%) to 85.524% when pulling 0be2a16 on vitorkusiaki:dataset-all-update into d53d3f3 on datasciencebr:master.

@jtemporal
Copy link
Collaborator

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.

@vitorkusiaki
Copy link
Contributor Author

@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?

@lipemorais
Copy link
Contributor

@vitorkusiaki, first some pending reviews need to be addressed on #132 before merge this one.

@jtemporal
Copy link
Collaborator

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:

  1. Running the tests to see if everything is passing
  • creating a new branch locally: git checkout -b vitorkusiaki-dataset-all-update master
  • pulling the changes proposed in this PR: git pull https://github.com/vitorkusiaki/serenata-toolbox.git dataset-all-update
  • Running tests: python -m unittest discover tests/unit
  1. Since this PR will mostly benefit other repositories setups I used serenata-de-amor environment to test this:
  • Edited the 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
  • with serenata_de_amor environment activated, ran the setup which gave me the following error:

screen shot 2017-08-25 at 8 25 19 pm

  • did the changed proposed in the review from #132 inside the source file locally
  • removed the toolbox line from requirements.txt to avoid overwriting the previous "hack"
  • ran setup again and everything works


@patch('os.path.exists')
@patch('serenata_toolbox.datasets.Datasets')
def test_fetch_latest_backup_with_force_all(self, datasets, os_path_exists):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this test also be added to journey tests?

cc @cuducos @lipemorais

Copy link
Contributor

Choose a reason for hiding this comment

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

@jtemporal since the dependencies are all mocked I believe that it's ok have this test under unit test folder. 👍

Copy link
Contributor

@lipemorais lipemorais left a comment

Choose a reason for hiding this comment

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

@jtemporal since the dependencies are all mocked I believe that it's ok have this test under unit test folder. 👍

@jtemporal
Copy link
Collaborator

hey @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 okfn-brasil/serenata-de-amor#273 to be merged!

Let me know if I can do anything to help out 😉

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 58.416% when pulling 29c8271 on vitorkusiaki:dataset-all-update into 3bb7358 on datasciencebr:master.

@vitorkusiaki
Copy link
Contributor Author

vitorkusiaki commented Oct 2, 2017

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

@lipemorais
Copy link
Contributor

Hey, @vitorkusiaki! There are some conflicts here with master branch. Could you fix this so we will be able to merge.

User can download everything if he wants, instead of only the missing
files.
The variable used in the for statement was f
@vitorkusiaki
Copy link
Contributor Author

It was a version conflict. It is fixed, @lipemorais. Thanks, hadn't noticed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 58.416% when pulling ba8250d on vitorkusiaki:dataset-all-update into 6179a2e on datasciencebr:master.

@cuducos cuducos merged commit cee4491 into okfn-brasil:master Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants