okfn-brasil / serenata-toolbox

📦 pip module containing code shared across Serenata de Amor's projects | ** Este repositório não recebe atualizações frequentes **
MIT License
154 stars 69 forks source link

Improve project health #147

Closed giovanisleite closed 7 years ago

giovanisleite commented 7 years ago

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

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 commented 7 years ago

Coverage Status

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

lipemorais commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Coverage Status

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

giovanisleite commented 7 years ago

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)

coveralls commented 7 years ago

Coverage Status

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

coveralls commented 7 years ago

Coverage Status

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