Improve project health #147

giovanisleite · 2017-09-23T22:23:36Z

What is the purpose of this Pull Request?
Improve the project health

What was done to achieve this purpose?
Code smells pointed out by prospector was removed/fixed as like some styles that wasn't right according to pep8 style guide

How to test if it really works?
Running prospector -s veryhigh --max-line-length 120 serenata_toolbox, it was pointed out 60 problems, now it is showing 30 (details about later).
EDIT: now 22, thanks to @lipemorais and @cuducos

Who can help reviewing it?
@lipemorais and anybody who wants it =))

TODO

Remove all code smells showed by landscape
Remove all code styles pointed by prospector

why i didn't solve the last 30 remaining:
13 invalid-name "df" - I think the readability of "df" is just fine, (i don't have exp) but seems that it is a common practice (maybe change to what_this_df_is_about_df?)
1 invalid-name "s3" - it is literal I think.
1 invalid-name "fh" - I couldn't understand what it means, so I couldn't think a valid name for it.
The other messages was about "too few public methods" and "no self use, could be a function" probably smells for great changes, related to Issue #87.

Landscape pointed out two more things that prospector didn't, list comprehension over filter and map functions, I changed just a map function for readability (I think), list comprehension over filters make it verbose and not more readable (if it seems inconsistency, let me know).

Should I change bump version?

coveralls · 2017-09-23T23:48:56Z

Coverage increased (+0.6%) to 58.14% when pulling 7c8c918 on giovanisleite:master into d6501e2 on datasciencebr:master.

lipemorais · 2017-09-24T01:54:53Z

1 invalid-name "fh" - I couldn't understand what it means, so I couldn't think a valid name for it.

I believe that @cuducos may help us with it.
I believe that it should be just f since it is the file read by aiofiles.read.

Should I change bump version?

Yeah we are trying to have a at least a minor bump version for any change in this project. :)

cuducos

Once more, many thanks for the contribution @giovanisleite!

IMHO some changes you made are inconsistent in terms of style. I commented on them inline.

Regarding some comments you posted:

1 invalid-name "fh" - I couldn't understand what it means, so I couldn't think a valid name for it.

fh is usually adopted in the Python community as an abbreviation for file handler. IMHO you can replace it by handler for example — if you want. Anyway personally I'm fine with fh.

The other messages was about "too few public methods" and "no self use, could be a function" probably smells for great changes, related to Issue #87.

When did you get the too few public methods? Probably they can be fixed with the @staticmethod decorator. Have you tried? — IMHO this bit doesn't have to do with #87, but the other warning has ; )

Should I change bump version?

Sure thing (thanks for the heads up @lipemorais)

cuducos · 2017-09-25T14:59:36Z

serenata_toolbox/federal_senate/dataset.py

-                print("While fetching, Seranata Toolbox didn't find file: {} \n{}".format(file_path, http_error_exception))
+                print("While fetching, Seranata Toolbox didn't find file: {} \n{}".format(
+                    file_path,
+                    http_error_exception))


It feels like you missed one space, and we have a matter os style:

Either…

print("While fetching, Seranata Toolbox didn't find file: {} \n{}".format(file_path, http_error_exception))

…or…

print("While fetching, Seranata Toolbox didn't find file: {} \n{}".format( file_path, http_error_exception) )

cuducos · 2017-09-25T14:59:43Z

serenata_toolbox/chamber_of_deputies/dataset.py

+        datasets_format = 'datasets-format.html'
+        url = (
+            'http://www2.camara.leg.br/transparencia/cota-para-exercicio-da-atividade-parlamentar/'
+            'explicacoes-sobre-o-formato-dos-arquivos-xml')


Style, either…

url = ( 'part1' 'part2' )

…or…

url = ('part1' 'part2')

cuducos · 2017-09-25T15:00:23Z

serenata_toolbox/federal_senate/dataset.py

-                print("While fetching, Seranata Toolbox didn't find file: {} \n{}".format(file_path, url_error_exception))
+                print("While fetching, Seranata Toolbox didn't find file: {} \n{}".format(
+                    file_path,
+                    url_error_exception))


It feels like you missed one space, and we have a matter os style:

Either…

print("While fetching, Seranata Toolbox didn't find file: {} \n{}".format(file_path, url_error_exception))

…or…

print("While fetching, Seranata Toolbox didn't find file: {} \n{}".format( file_path, url_error_exception) )

cuducos · 2017-09-25T15:03:33Z

serenata_toolbox/federal_senate/dataset.py

-                print("While translating, Seranata Toolbox didn't find file: {} \n{}".format(csv_path, file_not_found_error))
+                print("While translating, Seranata Toolbox didn't find file: {} \n{}".format(
+                    csv_path,
+                    file_not_found_error))


It feels like you missed one space, and we have a matter os style:

Either…

print("While translating, Seranata Toolbox didn't find file: {} \n{}".format(file_path, file_not_found_error))

…or…

print("While translating, Seranata Toolbox didn't find file: {} \n{}".format( file_path, file_not_found_error) )

cuducos · 2017-09-25T15:05:54Z

serenata_toolbox/federal_senate/dataset.py

+            ('Contratação de consultorias, assessorias, pesquisas, trabalhos técnicos e outros serviços'
+            ' de apoio ao exercício do mandato parlamentar'):
+                ('Recruitment of consultancies, advisory services, research, technical work and other services'
+                ' in support of the exercise of the parliamentary mandate'),


I think a better option would be to define two tuples, one with the pt-BR version, another with the en-US version and finally build the dictionary with something like categories = dict(zip(pt_br, en_us)).

cuducos · 2017-09-25T15:13:19Z

Ow… another doubt:

Remove all code smells showed by landscape
Remove all code styles pointed by prospector

prospector is the engine running in the backend of Landscape.io, so there should be no difference between these reports. If is there probably we have something missing in our .landscape.yml.

coveralls · 2017-09-25T23:58:34Z

Coverage increased (+0.8%) to 58.278% when pulling d2c4622 on giovanisleite:master into d6501e2 on datasciencebr:master.

giovanisleite · 2017-09-26T00:04:07Z

When did you get the too few public methods? Probably they can be fixed with the @staticmethod decorator. Have you tried? — IMHO this bit doesn't have to do with #87, but the other warning has ; )

pylint complains about classes having less than two public methods and the datasets classes have just fetch as public.
The other complaim, about the no self use, was resolved with your tip, thanks.

prospector is the engine running in the backend of Landscape.io, so there should be no difference between these reports. If is there probably we have something missing in our .landscape.yml.

In fact, on Landspcape.io appeared some "use list comprehension over map and filter" that prospector doesn't show up.

I removed the test that was testing the size of the resultset (thanks to discussion with @rennerocha on telegram)

…ymore

coveralls · 2017-09-26T01:29:14Z

Coverage increased (+28.6%) to 86.093% when pulling ace718d on giovanisleite:master into d6501e2 on datasciencebr:master.

cuducos

IMHO removing a test should not be part of a PR targeting code style and health reports. What do you think?

cuducos · 2017-09-26T13:03:16Z

tests/journey/test_chamber_of_deputies_official_missions_dataset.py

@@ -20,7 +20,6 @@ def test_fetch(self):
            'canceled', 'report_status', 'report_details_link'
        ]
        self.assertTrue(np.array_equal(expectedColumns, actualColumns))
-        self.assertEqual(56, len(df))


I totally agree that this test is a problem per se. However just for maintainability sake and for the community to follow the work on the project I'm not in favor of “smuggling” this change in the middle of a PR related to something else (code quality, health etc.).

There is a greater discussion about the role of tests in the project going on.

Would you mind reverting this change and let this to another PR/discussion?

BTW your commits are larger than I would recommend: not in terms of amount of changes, but in the purpose of the changes committed:

ace718d: identified static methods and removed test that doesn't make sense anymore

d2c4622: fixed spaces according to pep8 e128 and updated minor patch

In the first one listed you committed this change together with another unrelated change, so you cannot use git revert or git rebase to fix that. In the second is some other PR gets merged first, you cannot use git to revert a version bump… In general my advice is: if your commit message is X and Y you should commit twice: first commit X and then commit Y ; )

I do agree. :)

I removed the test with the thought that I was removing unused code as part of the project health. But I agree, it was bad and it is not consistent with others changes.

Oh, that's true. The commits are terrible.

I will fix that and come back
Thank you and srry for the trouble, like I told @lipemorais, the project that is contributing to me hahaha

coveralls · 2017-09-27T05:03:04Z

Coverage increased (+0.8%) to 58.278% when pulling 78c5499 on giovanisleite:master into d6501e2 on datasciencebr:master.

giovanisleite added 5 commits September 22, 2017 21:08

eliminate smells

0eefc6d

removing smells and fixing some styles according to pep8

aea2d69

break too long lines

4ee6f68

list comprehension over map

5467fda

fix broken strings

7c8c918

cuducos requested changes Sep 25, 2017

View reviewed changes

fixed spaces according to pep8 e128 and updated minor patch

d2c4622

identified static methods and removed test that doesn't make sense an…

ace718d

…ymore

cuducos requested changes Sep 26, 2017

View reviewed changes

giovanisleite added 7 commits September 27, 2017 00:57

Revert to organize my commits

ee5bd85

Removed unused imports

4be5a97

Add decorator to identify static methods

d3aec6e

Replaced variable invalid names to valid ones

2fba742

Fixed too long lines and extra spaces

4f7fb84

Solved minor problems

e1b1ab8

bumping version

78c5499

cuducos approved these changes Sep 27, 2017

View reviewed changes

cuducos merged commit 3bb7358 into okfn-brasil:master Sep 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve project health #147

Improve project health #147

giovanisleite commented Sep 23, 2017 •

edited

Loading

coveralls commented Sep 23, 2017

lipemorais commented Sep 24, 2017

cuducos left a comment

cuducos Sep 25, 2017

cuducos Sep 25, 2017

cuducos Sep 25, 2017

cuducos Sep 25, 2017

cuducos Sep 25, 2017

cuducos commented Sep 25, 2017

coveralls commented Sep 25, 2017

giovanisleite commented Sep 26, 2017 •

edited

Loading

coveralls commented Sep 26, 2017 •

edited

Loading

cuducos left a comment

cuducos Sep 26, 2017

lipemorais Sep 26, 2017

giovanisleite Sep 26, 2017

coveralls commented Sep 27, 2017

Improve project health #147

Improve project health #147

Conversation

giovanisleite commented Sep 23, 2017 • edited Loading

coveralls commented Sep 23, 2017

lipemorais commented Sep 24, 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

cuducos commented Sep 25, 2017

coveralls commented Sep 25, 2017

giovanisleite commented Sep 26, 2017 • edited Loading

coveralls commented Sep 26, 2017 • edited Loading

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

coveralls commented Sep 27, 2017

giovanisleite commented Sep 23, 2017 •

edited

Loading

giovanisleite commented Sep 26, 2017 •

edited

Loading

coveralls commented Sep 26, 2017 •

edited

Loading