test: add type hints and `mypy` linting #239

faissaloux · 2022-09-07T17:33:16Z

Hello 👋🏻

On this PR I have added type hints ✨
resolves #232

google-cla · 2022-09-07T17:33:19Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

lvaylet · 2022-10-06T19:22:32Z

Hi @faissaloux and thanks a lot for your contribution. I like type hints too. They make Python feel a little less interpreted and a little more statically typed. I will definitely take a look at this PR once I am done with adding the Cloud Monitoring MQL backend. Some commits I made there will make our life easier when it comes to linting and testing.

lvaylet

Thanks a lot for these annotations and the new MyPy target in the Makefile. I added a couple of suggestions for improvement.

slo_generator/backends/cloud_service_monitoring.py

mypy.ini

.pylintrc

slo_generator/api/main.py

slo_generator/backends/cloud_service_monitoring.py

slo_generator/exporters/cloudevent.py

.pylintrc

slo_generator/backends/cloud_service_monitoring.py

lvaylet

A lot of comments but not so many changes to make at the end of they day :-)

Makefile

mypy.ini

slo_generator/backends/prometheus.py

slo_generator/utils.py

faissaloux · 2022-10-15T23:37:52Z

@lvaylet heeey sorry for late response 😅 I've got busy
I just found some time so I can check your comments now 😃

lvaylet

Looking much better now! Just a few more changes and we should be good to go.

Also check the output of the CI for a handful of changes to make on Optional[str] arguments: https://github.com/google/slo-generator/actions/runs/3259133646

Finally, no need to apologize for being busy. This is an open source project and I am really happy you take some personal time to work on it. No pressure.

slo_generator/backends/prometheus.py

slo_generator/exporters/prometheus_self.py

slo_generator/report.py

faissaloux · 2022-10-16T22:17:09Z

About the Optional arguments, I think we should bring back the Optional type so the lint tests can pass.

What do you think!

lvaylet

Thanks a lot. I like how this looks.

You are right about bringing back Optional for the checks. Just add from typing import Optional at the top of the file, then a few Optional[str] type hints when it makes sense, as you did for Dict, Tuple, List...

.gitignore

Makefile

lvaylet

Just a couple small changes left.

Also, please stick to the original code as much as possible. I do not fully understand the purpose of the new code you introduced in main.py or other files (though I am sure there is a purpose :-)). Let's focus on type hints in this PR.

Makefile

slo_generator/api/main.py

slo_generator/backends/cloud_monitoring_mql.py

slo_generator/backends/cloud_service_monitoring.py

slo_generator/backends/prometheus.py

slo_generator/compute.py

faissaloux · 2022-10-17T20:26:26Z

Oow yeah you right I wanted to but I found my self changing it because lint keeps failing so I have to fix it 👀

About main.py I've got None has no attribute get so I made sure config is not None before getting anything from it .. or should I reverse it despite of the failing lint!!

Now we still have some errors because of some duplicated code 😐

ocervell

Looks good overall, thank you so much for taking the time to do this ! It will improve the dev experience and possibly ease-up some testing as well.

Some changes required, after that LGTM :)

lvaylet

Thanks Faissal. Just restore the original code for self.client in cloud_monitoring_mql.py and we should be good to go.

Regarding the linting checks that fail, you can add # pylint: disable=duplicate-code comments just above the definitions of good_bad_ratio() and distribution_cut() in cloud_monitoring.py. Then you can check that everything works from your local machine by running make lint. No need to wait for the CI pipeline to trigger.

slo_generator/backends/cloud_monitoring_mql.py

lvaylet

Good job. Now we need to work on making the pytype tests pass. Proceed incrementally, starting with the error in api/main.py mentioned below in a comment. Then make sure to run make lint on your local machine before pushing. Ideally this is the last review. I wll proceed with merging this PR if all checks succeed.

slo_generator/utils.py

lvaylet

LGTM, thanks a lot @faissaloux!

* add type hints * add more type hints * fix lint * cleanup * integrate mypy test to tests workflow * fix types test workflow * disable more pylint options * add new line to mypy.ini * disable warnings locally * change Pylint annotation position * update .pylintrc * fix misc errors * feat: add pytype linting (#249) * add pytype linting and fix all reported errors * revert metaclass fix to make tests pass * refactor Makefile * fix mypy errors * more specific types in prometheus.py * remove Optional type on arguments with default * update test workflow * fix lint * fix unsubscriptable-object errors * refactor variables declarations and some typings * add new line on .gitignore file * remove clean from mypy make cmd * keep line before cmd * bring back Optional type * bring back Optional type on utils.py * reverse last commit * add Optional type to args in utils.py * wrong arg type * fix annotation-type-mismatch in compute.py * fix attribute-error on main.py * fix line too long * fix indentation * fix return types * add Optional type * fix already defined variables * fix None has no attribute * fix line too long * update Makefile mypy script * reverse last changes * reverse cloud_monitoring_mql init refactoring * ignore lint duplicate code warnings * skip pytype attribute-error error to fix later * ignore attr-defined type errors * ignore union-attr type errors * ignore fixme pylint warning * add missing types dependencies Co-authored-by: Laurent Vaylet <[email protected]>

faissaloux marked this pull request as ready for review September 7, 2022 19:55

lvaylet self-requested a review October 6, 2022 19:22

lvaylet requested changes Oct 7, 2022

View reviewed changes

slo_generator/backends/cloud_service_monitoring.py Outdated Show resolved Hide resolved

mypy.ini Outdated Show resolved Hide resolved

faissaloux requested a review from lvaylet October 7, 2022 11:37

lvaylet requested changes Oct 7, 2022

View reviewed changes

.pylintrc Show resolved Hide resolved

slo_generator/api/main.py Outdated Show resolved Hide resolved

slo_generator/backends/cloud_service_monitoring.py Show resolved Hide resolved

slo_generator/exporters/cloudevent.py Show resolved Hide resolved

faissaloux requested a review from lvaylet October 7, 2022 12:29

lvaylet requested changes Oct 7, 2022

View reviewed changes

.pylintrc Outdated Show resolved Hide resolved

slo_generator/backends/cloud_service_monitoring.py Outdated Show resolved Hide resolved

faissaloux requested a review from lvaylet October 7, 2022 13:12

lvaylet changed the title ~~Add type hints~~ test: add type hints and mypy linting Oct 13, 2022

lvaylet assigned faissaloux Oct 13, 2022

lvaylet added the tests Testing improvements / bugs label Oct 13, 2022

lvaylet requested changes Oct 13, 2022

View reviewed changes

faissaloux force-pushed the type-hints branch from d611a54 to 2b3aa87 Compare October 16, 2022 00:42

faissaloux requested review from lvaylet and removed request for lvaylet October 16, 2022 01:26

faissaloux force-pushed the type-hints branch 2 times, most recently from a99f30d to 4a8452c Compare October 16, 2022 10:24

lvaylet requested changes Oct 16, 2022

View reviewed changes

faissaloux requested a review from lvaylet October 16, 2022 22:14

lvaylet requested changes Oct 17, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

faissaloux force-pushed the type-hints branch from 12962d4 to 452152f Compare October 17, 2022 16:53

faissaloux requested a review from lvaylet October 17, 2022 18:52

lvaylet requested changes Oct 17, 2022

View reviewed changes

ocervell requested changes Oct 19, 2022

View reviewed changes

faissaloux added 14 commits October 22, 2022 01:47

reverse last commit

d10b9e2

add Optional type to args in utils.py

56a38a3

wrong arg type

1574a3e

fix annotation-type-mismatch in compute.py

04e5998

fix attribute-error on main.py

c863cf6

fix line too long

5f0e3d5

fix indentation

d5f385f

fix return types

46dcfca

add Optional type

9c5bee1

fix already defined variables

d312dea

fix None has no attribute

3d482dd

fix line too long

f57569b

update Makefile mypy script

c0ad4fb

reverse last changes

b3c6357

faissaloux force-pushed the type-hints branch from 128598b to b3c6357 Compare October 22, 2022 00:48

lvaylet requested changes Oct 22, 2022

View reviewed changes

slo_generator/backends/cloud_monitoring_mql.py Outdated Show resolved Hide resolved

reverse cloud_monitoring_mql init refactoring

da558ec

faissaloux requested a review from lvaylet October 22, 2022 11:03

ignore lint duplicate code warnings

365a089

lvaylet requested changes Oct 22, 2022

View reviewed changes

slo_generator/utils.py Show resolved Hide resolved

faissaloux added 5 commits October 23, 2022 01:54

skip pytype attribute-error error to fix later

f2b3e2f

ignore attr-defined type errors

c035ad3

ignore union-attr type errors

88718fd

ignore fixme pylint warning

e5ad5a9

add missing types dependencies

e3dad1d

faissaloux requested a review from lvaylet October 23, 2022 02:55

lvaylet approved these changes Oct 23, 2022

View reviewed changes

lvaylet merged commit 44d6445 into google:master Oct 23, 2022

faissaloux deleted the type-hints branch October 23, 2022 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add type hints and `mypy` linting #239

test: add type hints and `mypy` linting #239

faissaloux commented Sep 7, 2022 •

edited

Loading

google-cla bot commented Sep 7, 2022

lvaylet commented Oct 6, 2022

lvaylet left a comment

lvaylet left a comment

faissaloux commented Oct 15, 2022

lvaylet left a comment

faissaloux commented Oct 16, 2022

lvaylet left a comment •

edited

Loading

lvaylet left a comment

faissaloux commented Oct 17, 2022

ocervell left a comment

lvaylet left a comment

lvaylet left a comment

lvaylet left a comment

test: add type hints and mypy linting #239

test: add type hints and mypy linting #239

Conversation

faissaloux commented Sep 7, 2022 • edited Loading

google-cla bot commented Sep 7, 2022

lvaylet commented Oct 6, 2022

lvaylet left a comment

Choose a reason for hiding this comment

lvaylet left a comment

Choose a reason for hiding this comment

faissaloux commented Oct 15, 2022

lvaylet left a comment

Choose a reason for hiding this comment

faissaloux commented Oct 16, 2022

lvaylet left a comment • edited Loading

Choose a reason for hiding this comment

lvaylet left a comment

Choose a reason for hiding this comment

faissaloux commented Oct 17, 2022

ocervell left a comment

Choose a reason for hiding this comment

lvaylet left a comment

Choose a reason for hiding this comment

lvaylet left a comment

Choose a reason for hiding this comment

lvaylet left a comment

Choose a reason for hiding this comment

test: add type hints and `mypy` linting #239

test: add type hints and `mypy` linting #239

faissaloux commented Sep 7, 2022 •

edited

Loading

lvaylet left a comment •

edited

Loading