Add command line option to limit dataset years #115

anaschwendler · 2017-12-08T17:12:25Z

This is just a template to help you make your point clear with this PR. :)

What is the purpose of this Pull Request?
Following @Irio #60 with the option to limit dataset years, this PR:

Update the way to select an specific path to save the datasets
Fetch the Dataset() method for year, only if years is no None
Update the README.md to this new option

What was done to achieve this purpose?
Understand what @Irio has done to achieve that new option and fix what was missing to make this new option works

How to test if it really works?
Run rosie with a few options like:

$ python rosie.py run chamber_of_deputies --path /another/folder --years 2017
$ python rosie.py run chamber_of_deputies --path /another/folder
$ python rosie.py run chamber_of_deputies --years 2017,2016
$ python rosie.py run chamber_of_deputies 

$ python rosie.py run federal_senate --path /another/folder --years 2017
$ python rosie.py run federal_senate --path /another/folder
$ python rosie.py run federal_senate --years 2017,2016
$ python rosie.py run federal_senate

Who can help reviewing it?
@cuducos

@Irio

Following @Irio PR with the option to limit dataset years, this PR: - Update the way to select an specific path to save the datasets - Fetch the `Dataset()` method for year, only if `years` is no None - Update the README.md to this new option

anaschwendler · 2017-12-08T17:22:25Z

There will be some tests to add to rosie, but I think that it is a great start!

cuducos · 2017-12-08T17:52:54Z

There will be some tests to add to rosie, but I think that it is a great start!

Sure it is! Many thanks for that ; )

Two doubts:

Should we wait for these tests before reviewing the PR?
As the CLI gets more complicates I think we should adopt either argparse (⭐️ ), click (⭐️ ⭐️⭐️⭐️) or docopt ( ⭐️⭐️⭐️⭐️⭐️). What do you think of implementing that in this PR?

anaschwendler · 2017-12-08T17:58:26Z

Should we wait for these tests before reviewing the PR?

This feature is important, so I must consider accept as it is now, and open a new PR to that

As the CLI gets more complicates I think we should adopt either argparse (⭐️ ), click (⭐️ ⭐️⭐️⭐️) or docopt ( ⭐️⭐️⭐️⭐️⭐️). What do you think of implementing that in this PR?

I would like to do it now, but I think it is too much for one PR, but I can add too, or we can open an issue and I can do it following, what do you think @cuducos ?

cuducos · 2017-12-08T18:02:26Z

This feature is important, so I must consider accept as it is now, and open a new PR to that

If the feature is importante I'd say the opposite: let's not accept non-tested code ; ) Also, if the feature is important I think using argparse, click or docopt is very much welcomed: they are highly tested packages to avoid us reinventing the wheel and accidentally getting something wrong in in the way.

TLDR I honestly prefer tests and a high level CLI development tool before reviewing this PR bizarrely for the same reason you prefer to postpone tests and high level CI dev tool hahaha…

anaschwendler · 2017-12-08T18:03:41Z

Oh! Nice point, working on it!! \o\

anaschwendler · 2017-12-08T18:41:58Z

So considering what we need to do:

Test chamber_of_deputies with and without years
Include docopt to our CLI

anaschwendler · 2017-12-23T23:15:01Z

Now that I'm full recovered, I will get back to it!

anaschwendler · 2017-12-26T14:02:45Z

Just included docopt to our CLI, and had some doubts about documentation.

Docopt works fine, but when I run python rosie.py only, it gets a help > terminal, is it normal?

Independent of what I do, it don't change this option, what can be done about it?

anaschwendler · 2017-12-26T16:04:32Z

Just add the tests with and without years for chamber_of_deputies. The result is:

(serenata_rosie) ➜  rosie git:(anaschwendler-limit-years) ✗ python rosie.py test
..............................................................
----------------------------------------------------------------------
Ran 62 tests in 152.857s

OK

I'm completely sure that this it not the best way to do that, but I think that the logic is ok. I'll wait for @cuducos review, to see if he can help me with my tests.

cuducos

I think a better usage of docopt can considerably enhance rosie.py script.

What I suggest is as follows:

Removing argv altogether
Using docopt as soon as possible in the script execution to extract the meaningfull arguments
Simplify run and test so they don't deal with argument parsing
Call run or test after docopt is done with argument parsing

For example, on the bottom of the file:

def main():
    arguments = docopt(__doc__)

    if arguments.get('test'):
        return test(**arguments)

    if arguments.get('run'):
        return run(**arguments)


if __name__ == '__main__':
    main()

cuducos · 2018-01-04T08:23:34Z

rosie.py

+    python rosie.py test [chamber_of_deputies | federal_senate]
+
+Options:
+    --years   runs rosie wuth years filter


Typo: wuth -> with

Rosie is a proper noun and should be capitalized.

In years filter the word years is an adjective (I guess) and should not be pluralized.

Also runs rosie wuth years filter doesn't tell much about what years filter does. What about: runs Rosie fetching data and finding suspicions for a specific set of years?

accepted 👍

cuducos · 2018-01-04T08:31:21Z

README.md

+$ python rosie.py run chamber_of_deputies --path /my/serenata/directory/
+```
+
+Besides that, if you want to, you can run the project with years filter - for example:


Line 52 uses a m-dash (instead of an hyphen) before for example. This line uses an hyphen. Either we go with m-dash or with hyphen, but swinging between both is weird.

Also a line break between text and mono-spaced block makes the raw markdown way easier to read.

cuducos · 2018-01-04T08:34:09Z

rosie.py

 from sys import argv, exit
+from docopt import docopt


PEP8 suggests a blank line between native modules (sys) and third-party modules (docopt).

cuducos · 2018-01-04T08:37:53Z

rosie.py

+
+Options:
+    --years   runs rosie wuth years filter
+    --path    runs rosie with specific directory


Rosie is a proper noun and should be capitalized.

Also runs runs rosie with specific directory doesn't tell much about what this directory is for What about: directory in which Rosie saves fetched data and the suspicions CSV?

If I'm not wrong this optional argument has a default (data/? /tmp/serenata-data/?) that should be documented as well.

If I'm not wrong this optional argument has a default (data/? /tmp/serenata-data/?) that should be documented as well.

Yes, serenata-data, can you help me documenting that? There is something in the README.md but I guess we can do more.

can you help me documenting that?

From the snippet of docopt docs I pasted yesterday it's is as simple as replacing this very line we're commenting on. Just change:

--path runs rosie with specific directory

by:

--path runs rosie with specific directory [default: /tmp/serenata-data]

cuducos · 2018-01-04T08:40:53Z

rosie.py

        exit(1)
-    target_directory = argv[3] if len(argv) >= 4 else '/tmp/serenata-data/'
+
+    target_directory = argv[argv.index('--path') + 1] if '--path' in argv else '/tmp/serenata-data/'


We should use docopt for the default value too. From their docs:

If you want to set a default value for an option with an argument, put it into the option's description, in the form [default: <the-default-value>].

cuducos · 2018-01-04T08:45:18Z

rosie/chamber_of_deputies/tests/test_adapter.py

+        years = AVAILABLE_YEARS
+        chamber_of_deputies = Dataset(self.temp_path, years)
+        datasets = chamber_of_deputies.fetch()
+        self.assertEqual(10, len(datasets))


This hard-coded 10 will break tests every new year. len(AVAILABLE_YEARS) wont ; )

accepted, I just didn't know we could do that

cuducos · 2018-01-04T08:45:58Z

rosie/chamber_of_deputies/tests/test_adapter.py

+        self.assertEqual(10, len(datasets))
+
+    def test_filter_years_fetch(self):
+        years = [2017,2016]


PEP8 recommends a space after this comma.

cuducos · 2018-01-04T08:47:18Z

rosie/chamber_of_deputies/tests/test_adapter.py

+        years = [2017,2016]
+        chamber_of_deputies = Dataset(self.temp_path, years)
+        datasets = chamber_of_deputies.fetch()
+        self.assertEqual(3, len(datasets))


Why we expect 3 when len(years) == 2? Can we document that? For example:

self.assertEqual(3, len(datasets)) # data from 2 years + suspicions file

cuducos · 2018-01-04T08:49:41Z

rosie.py

+    --path    runs rosie with specific directory
+
+"""
+
 from sys import argv, exit


The idea of docopt is to avoid using argv (exit is still needed for the test command).

cuducos · 2018-01-04T08:52:55Z

rosie.py

 from sys import argv, exit
+from docopt import docopt


 def entered_command(argv):


We don't need this method when using docopt.

anaschwendler · 2018-01-05T19:18:18Z

Hey @cuducos I think I got something here :)

I'm sure that there is a few things to fix, but its getting better, can you review for me?

anaschwendler · 2018-01-05T19:18:58Z

rosie.py

-    target_directory = argv[3] if len(argv) >= 4 else '/tmp/serenata-data/'
+    target_directory = args['<path>'] if args['--path'] else '/tmp/serenata-data/'
+
+    target_module = 'chamber_of_deputies' if args['chamber_of_deputies'] else 'federal_senate'


@cuducos I'm not into this way of selecting target_module do you have an better option for that?

When using docopt we should have in arguments a dictionary like this:

{'--path': False, '--years': False, 'chamber_of_deputies': True, 'core': False, 'federal_senate': False, 'run': True, 'test': False}

So I'd go for something like:

commands = ('chamber_of_deputies', 'federal_senate') target_module, *_ = filter(lambda x: arguments.get(x), commands)

cuducos

@anaschwendler I tried to improve the docopt description using their online tool – the output I;ve got with your code was too polluted so I guesses that something was wrong. I polished it with attention to details (line breaks, syntax etc) and I figure out minor details that could be improved and simplified. Take a look and check that writing this description properly helps us to have a neat arguments dictionary.

This description:

Hi, this is Serenata's Rosie!

Usage:
  rosie.py run (chamber_of_deputies | federal_senate) [--path=<path>] [--years=<years>]
  rosie.py test [chamber_of_deputies | federal_senate]

Options:
  --years=<years>  runs Rosie fetching data and finding suspicions for a specific set of years (use a comma to separate years)
  --path=<path>    directory in which Rosie saves fetched data and the suspicions CSV [default: /tmp/serenata-data]

Gives us this output:

{
  "--path": "/tmp/serenata-data", 
  "--years": None, 
  "chamber_of_deputies": True, 
  "federal_senate": False, 
  "run": True, 
  "test": False
}

My following comments will take into account this dictionary as our arguments, ok?

cuducos · 2018-01-06T13:33:38Z

rosie.py

-        help()
-        exit(1)
-    target_directory = argv[3] if len(argv) >= 4 else '/tmp/serenata-data/'
+    target_directory = args['<path>'] if args['--path'] else '/tmp/serenata-data/'


No need to set the default value: docopt will fill in that for you if not entered. target_directory = args['--path'] should be enough. Or move on directly with args['--path'] ; )

I knew that should be more improvements, thanks for that, looks pretty good :)

cuducos · 2018-01-06T14:08:48Z

rosie.py

+        years = [int(num) for num in years.split(',')]
+        klass.main(target_directory, years=years)
+    else:
+        klass.main(target_directory)


No need for this if/else here since:

if args['--years'] is None

and considering that the default value for years is None (and it is)

then klass(target_dir) is exactly equal to klass(target_dir, args['--years']

However I'm afraid we might need to convert the string 2016,2017 to a list of integers, i.e. [2016, 2017]. For example:

if args['--years']: args['--years'] = [int(v) for v in args['--years'].split(',')] klass.main(target_directory, args['--years'])

cuducos · 2018-01-06T14:13:32Z

rosie.py

+        tests_path = os.path.join('rosie', 'chamber_of_deputies')
+        tests = loader.discover(tests_path)
+    elif args['federal_senate']:
+        tests_path = os.path.join('rosie', 'federal_senate')
        tests = loader.discover(tests_path)
    else:
        tests = loader.discover('rosie')


Minor DRY suggestion. What about:

test_path = 'rosie' if args['chamber_of_deputies']: tests_path = os.path.join(test_path, 'chamber_of_deputies') elif args['federal_senate']: tests_path = os.path.join(test_path, 'federal_senate') tests = loader.discover(test_path)

anaschwendler · 2018-01-07T21:11:25Z

Hi @cuducos took a closer look on what you have said, and made some adjustments. I'm still not happy with the test script. But now the main script I think that may look nice. Can you review again for me?

cuducos · 2018-01-07T23:05:14Z

rosie/chamber_of_deputies/tests/test_adapter.py

@@ -27,6 +30,18 @@ def setUp(self):
    def tearDown(self):
        shutil.rmtree(self.temp_path)

+    def test_quantity_of_downloaded_datasets_all_years_fetch(self):
+        years = AVAILABLE_YEARS


Why do we need this? You use years in line 35 but go back to AVAILABLE_YEARS in 37.

didn't noticed, thanks /o/

cuducos · 2018-01-07T23:05:54Z

I'm still not happy with the test script.

What bugs you in this script?

anaschwendler · 2018-01-07T23:09:26Z

@cuducos It takes time by downloading the datasets from the source, I can't think of anything, but maybe there is another way to test that, without downloading the whole datasets

cuducos · 2018-01-07T23:13:43Z

there is another way to test that, without downloading the whole datasets

You could mock HTTP requests, or you could mock only the classes (as internally the way self.years works might be already tested).

Irio and others added 6 commits June 18, 2017 03:52

Add command line option to limit dataset years

dca576e

Merge branch 'master' into irio-limit-years

9cca1fe

Fix command for running without specifying year list

7a63338

Add years parameter to federal_senate module

f6af363

Merge branch 'master' into irio-limit-years

5d9bb16

Enhancement to the years definition, thanks @cuducos

9133a14

anaschwendler force-pushed the anaschwendler-limit-years branch 3 times, most recently from 981abff to fc420ec Compare December 8, 2017 18:46

Fix attempt to adapt tests

a7809ca

anaschwendler force-pushed the anaschwendler-limit-years branch from fc420ec to a7809ca Compare December 8, 2017 18:47

anaschwendler added 2 commits December 26, 2017 11:08

Merge branch 'master' into anaschwendler-limit-years

2dd59f2

Include docopt to our CLI

c3303f3

Test Chamber of Deputies with and without years

0663633

anaschwendler requested a review from cuducos December 26, 2017 16:04

cuducos suggested changes Jan 4, 2018

View reviewed changes

anaschwendler added 3 commits January 5, 2018 12:09

Uses m-dash in both for example;include a line break and the code block

991a562

Enhancing tests

c15c84a

Adjust rosie.py with docopt

f50c9d1

anaschwendler commented Jan 5, 2018

View reviewed changes

Enhance target_module selection

37336e5

cuducos suggested changes Jan 6, 2018

View reviewed changes

Documentation and script enhancements

11c4900

cuducos reviewed Jan 7, 2018

View reviewed changes

Add command line option to limit dataset years #115

Are you sure you want to change the base?

Add command line option to limit dataset years #115

Conversation

anaschwendler commented Dec 8, 2017

anaschwendler commented Dec 8, 2017

cuducos commented Dec 8, 2017

anaschwendler commented Dec 8, 2017

cuducos commented Dec 8, 2017

anaschwendler commented Dec 8, 2017

anaschwendler commented Dec 8, 2017 • edited Loading

anaschwendler commented Dec 23, 2017

anaschwendler commented Dec 26, 2017

anaschwendler commented Dec 26, 2017

cuducos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anaschwendler commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuducos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anaschwendler commented Jan 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuducos commented Jan 7, 2018

anaschwendler commented Jan 7, 2018

cuducos commented Jan 7, 2018

anaschwendler commented Dec 8, 2017 •

edited

Loading

anaschwendler commented Jan 7, 2018 •

edited

Loading