Enable mypy pre-commit hook #2674

VannTen · 2022-08-08T15:51:33Z

see #2673

mayaCostantini

lgtm, thanks! :100

sesheta · 2022-08-09T07:03:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mayaCostantini

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mayaCostantini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

VannTen · 2022-08-09T07:08:05Z

I only enabled the hook, still needs to gruntily fix all the errors ^

Having the config in pyproject.toml make it usable by other tools, notably IDE.

VannTen · 2022-08-18T15:25:54Z

Related to the errors in thoth/storages/graph/models_base.py:
https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html

TL;DR (from my understanding)typing support is not super practical in SQLAlchemy
declarative API in 1.3/1.4. Either needs a mypy plugin (which is alpha and
deprecated for sqlalchemy 2.0) or switching to 2.0 (in dev).

VannTen · 2022-08-23T15:25:15Z

Note: in 2e5cd41 I explicited what I assumed to be the intent.

VannTen · 2022-09-08T10:27:40Z

The graph/* errors are a bit hard to fix without restructuring quite intensely
the code, so I think we could benefits from merging this as is. (Finding a way
to exclude thoth.storages.graph module would be great, but for now I don't know
how (see python/mypy#10820))

For reviewers: I think most of the commits are reviewable individually. In fact
they can probably be cherry-picked individually as well.

Some are "breaking" changes to the API, (in the types) but in most cases the
underlying implementation was already breaking stuff (or the call sites used the
code correctly despite wrong types ! ^).

I will detail the commits changing the API in a later comment.

VannTen · 2022-09-14T09:09:11Z

Commits breaking API.

6060262 Fix incompatible method overload for AnalysisByDigest
Only call site has a pending PR
solvers:
590a8ab Fix incompatible child class method override
ab70c50 Use TypedDict for reifying "all or nothing" for solver infos
-> Align the API + use types to states parameters requirements -> api change
c10b40b sync: More precise handlers types
Not breaking (List are Iterable)
3e6d529 sync: remove unused parameters and refactor
Align interface with behavior (removed parameters were not used)
6e7061b CephCache
Correct return type
ec8b10b dependency_monkeys
Correct return type
33a6bca Trying to fix get_document_ on solvers
This avoid the needs for override on get_document_count but kinda drop type safety for it.
Best option would be to make it a static method / utils and feed it the result of get_document_listing instead of including it in the class (since we don't do anything more efficient like using list_objects or something like that)

Note that this raises the minimal python version to 3.8

We use random.shuffle, but the import masked the random module with the random function.

This is a cleanup after 5d8b2bc, which removed amun + inspection_only stuff from sync functions, but not from the sync_documents "dispatcher" function. We also refactor the sync function for clarity.

- List -> Iterable (typing) Iterable is more flexible + match more closely the internal of the sync functions. - 4 ints tuples return instead of polymorphic

This is a breaking change of the API, but since it was broken (turning a string into a dict). fix thoth-station#2689

We need to rely on CI and the graph module is a bit wild for now. This should be temporary.

sesheta requested review from harshad16 and KPostOffice August 8, 2022 15:51

sesheta added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 8, 2022

mayaCostantini approved these changes Aug 9, 2022

View reviewed changes

sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2022

VannTen force-pushed the fix_mypy branch from b9a3056 to d20babd Compare August 17, 2022 13:55

VannTen added 2 commits August 17, 2022 15:55

Enable mypy and put its config in pyproject.toml

0f16910

Having the config in pyproject.toml make it usable by other tools, notably IDE.

mypy: exclude docs

83e2596

sesheta added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 17, 2022

VannTen mentioned this pull request Aug 22, 2022

Reevaluate custom lru_cache decorator #2688

Open

VannTen force-pushed the fix_mypy branch 3 times, most recently from ea806dd to 814e6de Compare August 24, 2022 09:09

This was referenced Aug 24, 2022

AnalysisByDigest intented usage #2689

Closed

Question: Using sqlalchemy-file (or similar) ? #2691

Open

VannTen force-pushed the fix_mypy branch from c250268 to ed19d9b Compare August 26, 2022 13:24

VannTen force-pushed the fix_mypy branch from 711233e to 0ca4164 Compare September 8, 2022 10:03

VannTen mentioned this pull request Sep 8, 2022

Modify AnalysisByDigest usage thoth-station/user-api#1805

Merged

VannTen force-pushed the fix_mypy branch 3 times, most recently from d1d1d0b to 525bb58 Compare September 14, 2022 09:08

VannTen force-pushed the fix_mypy branch from 525bb58 to 58db471 Compare September 14, 2022 13:08

S3Resource typing

76f3719

VannTen force-pushed the fix_mypy branch from 58db471 to 4178f0c Compare September 14, 2022 15:02

Use TypedDict for reifying "all or nothing" for solver infos

ab70c50

Note that this raises the minimal python version to 3.8

VannTen force-pushed the fix_mypy branch from 4178f0c to ae11d22 Compare October 10, 2022 10:03

VannTen added 15 commits October 10, 2022 12:04

Trying to fix get_document_* on solvers

33a6bca

dependency_monkeys

ec8b10b

CephCache

6e7061b

Fix type confusion (same var name, different types)

234e8d8

Fix bad import

0449a70

We use random.shuffle, but the import masked the random module with the random function.

sync: remove unused parameters and refactor

3e6d529

This is a cleanup after 5d8b2bc, which removed amun + inspection_only stuff from sync functions, but not from the sync_documents "dispatcher" function. We also refactor the sync function for clarity.

sync: More precise handlers types

c10b40b

- List -> Iterable (typing) Iterable is more flexible + match more closely the internal of the sync functions. - 4 ints tuples return instead of polymorphic

Fix type inconsistency with explicit conversion

8ded948

Fix incompatible child class method override

590a8ab

Remove unused code

0b45759

Proper typing for security indicators

6aa1eec

Light refactor on security_indicator utilities

0cd11dc

Fix incompatible method overload for AnalysisByDigest

6060262

This is a breaking change of the API, but since it was broken (turning a string into a dict). fix thoth-station#2689

Inspections: factorize constructor.

03b531a

Ignore storages.graph type errors

ae11d22

We need to rely on CI and the graph module is a bit wild for now. This should be temporary.

VannTen requested a review from mayaCostantini October 10, 2022 10:10

sesheta merged commit b939b9f into thoth-station:master Oct 10, 2022

mayaCostantini mentioned this pull request Oct 10, 2022

/approve label should be removed when new changes are detected AICoE/sefkhet-abwy#204

Closed

VannTen mentioned this pull request Nov 16, 2022

Correct typing in storages.graph #2735

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable mypy pre-commit hook #2674

Enable mypy pre-commit hook #2674

Uh oh!

VannTen commented Aug 8, 2022

Uh oh!

mayaCostantini left a comment

Uh oh!

sesheta commented Aug 9, 2022

Uh oh!

VannTen commented Aug 9, 2022 via email

Uh oh!

VannTen commented Aug 18, 2022

Uh oh!

VannTen commented Aug 23, 2022 •

edited

Loading

Uh oh!

VannTen commented Sep 8, 2022

Uh oh!

VannTen commented Sep 14, 2022 •

edited

Loading

Uh oh!

Uh oh!

Enable mypy pre-commit hook #2674

Enable mypy pre-commit hook #2674

Uh oh!

Conversation

VannTen commented Aug 8, 2022

Uh oh!

mayaCostantini left a comment

Choose a reason for hiding this comment

Uh oh!

sesheta commented Aug 9, 2022

Uh oh!

VannTen commented Aug 9, 2022 via email

Uh oh!

VannTen commented Aug 18, 2022

Uh oh!

VannTen commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VannTen commented Sep 8, 2022

Uh oh!

VannTen commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

VannTen commented Aug 23, 2022 •

edited

Loading

VannTen commented Sep 14, 2022 •

edited

Loading