Mentorship-backend: Add automated linting to tests and Travis

Created on 31 Dec 2018 · 15Comments · Source: anitab-org/mentorship-backend

Is your feature request related to a problem? Please describe.
As the codebase grows, it will be very important to ensure everyone follows the code style and standards in order to ensure consistency and maintainability.

Describe the solution you'd like
Integrate and configure automated linter checks in both local tests and as a part of Travis CI.

Quality Assurance Maintenance

Source

iulianav

👍1

All 15 comments

I understand very little about TravisCI configurations, could you explain better what we need for this?
cc @m-murad knows more about TravisCI things than I do

isabelcosta on 31 Dec 2018

@isabelcosta @m-murad it should be easy to do it! Just install a linter e.g. pylint in the virtual environment and ensure we are running it with the unit tests. I will investigate more. What do you think? Is it worth it? I know it is a good practice to have it and automatically ensure python standards are met in every PR.

iulianav on 2 Jan 2019

👍1

I think that any best practice, things that help us code and maintain code better are very welcome :) So I think it is worth it.
I'm sure when @m-murad becomes available he'll be able to help us with this :)

Thank you for suggesting this. If you have any other suggestion for this project, feel free to suggest more @iulianav :)

isabelcosta on 2 Jan 2019

@isabelcosta I forgot to mention: this will require some mass refactoring given that PEP8 standards are not respected in many parts of the project. I am happy to do it, but it might take a bit.

iulianav on 3 Jan 2019

@iulianav thank you for pointing that out. Can you tell me some of the PEP8 violations? Since I'm not a python developer by default, I don't catch these things.

isabelcosta on 3 Jan 2019

@isabelcosta an example of what the basic flake8 linter found:

app/api/api_extension.py:19:1: E402 module level import not at top of file
app/api/api_extension.py:19:80: E501 line too long (96 > 79 characters)
app/api/api_extension.py:21:50: W292 no newline at end of file

But it is mostly line lengths. We can ignore line length checks if you think it is too much tho.

iulianav on 3 Jan 2019

@iulianav I saw it now! There are a lot of violations of Travis from your PR. Now I see why this will need a lot of refactoring. Also, I noticed these violations:

app/api/api_extension.py:11:1: E402 module level import not at top of file
app/api/api_extension.py:15:1: E402 module level import not at top of file
app/api/api_extension.py:19:1: E402 module level import not at top of file

This worries me because there are some import's I do at the module level, that are necessary otherwise I have to change other things.

Would you take care of these mass refactoring? Once I approve your PR Travis will always fail due to PEP8 standards violations. Until those violations are solved, I'll have to take a closer look at the Travis output to see if tests pass even if standards fail.

cc @m-murad

isabelcosta on 3 Jan 2019

👍1

@isabelcosta I believe the imports check can be ignored if there is no other choice by configurating flake8. I have to look into it.

Regarding the refactoring, I could make the refactoring part of this PR too and have 2 commits: 1 for flake8, 1 for the refactoring?

iulianav on 3 Jan 2019

@isabelcosta I made flake8 ignore the E402 check:
i.e. app/api/api_extension.py:11:1: E402 module level import not at top of file.

iulianav on 5 Jan 2019

👍1

@iulianav Thank you! I hope that ignore action is ok. Because at the time that I coded that, I didn't see another way of importing on the top level, without circular imports.

isabelcosta on 6 Jan 2019

👍1

@SanketDG regarding this comment of yours if you wish to take this up of using flake8/pylint, I can assign this to you. Although now you would have to do this on GitHub Actions instead of TravisCI. Would you like me to assign this to you?

Thank you so much for your work @iulianav in this PR: https://github.com/anitab-org/mentorship-backend/pull/174

isabelcosta on 25 Jul 2020

@isabelcosta Sure, that's fine!

flake8 will detect a whole bunch of issues if it's added now, so we will have to also run a single pass of black as it was done in #465, and I will also be integrating an action so that every PR author has to run black on their code for the tests to pass! :tada:

Do you want me to do the black thing first and then make flake8 work with it?

SanketDG on 25 Jul 2020

@SanketDG yes, perhaps the black github action should come first in a specific PR and then the flake on another separate, by coming after, the flake run will pass right?

Can you create an issue for the black GH action and then us this one for the flake?

isabelcosta on 25 Jul 2020

yes, perhaps the black github action should come first in a specific PR and then the flake on another separate, by coming after, the flake run will pass right?

Yes!