Alternative mypy setup #3315
Alternative mypy setup #3315
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| def batch_insert_contributors(logger, data: Union[List[dict], dict]) -> Optional[List[dict]]: | ||
|
|
||
| batch_size = 1000 | ||
| def batch_insert_contributors(logger, data: Union[List[dict], dict], batch_size = 1000) -> Optional[List[dict]]: |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0621: Redefining name 'logger' from outer scope (line 19) (redefined-outer-name)
|
|
||
|
|
||
| def bulk_insert_dicts(logger, data: Union[List[dict], dict], table, natural_keys: List[str], return_columns: Optional[List[str]] = None, string_fields: Optional[List[str]] = None, on_conflict_update:bool = True) -> Optional[List[dict]]: | ||
| def bulk_insert_dicts(logger, data_input: Union[List[dict], dict], table, natural_keys: List[str], return_columns: Optional[List[str]] = None, string_fields: Optional[List[str]] = None, on_conflict_update:bool = True) -> Optional[List[dict]]: |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0621: Redefining name 'logger' from outer scope (line 19) (redefined-outer-name)
|
@skools-here what do you think of this? I made this branch mostly just to see what would happen if I ran mypy on augur out of the box (answer: a ton of type errors i didnt want to deal with) so I ended up narrowing it to the db files and only had like 6 things to fix. It turns out mypy generally won't complain if things are untyped, which seems helpful for migrating. The only thing left IMO is to figure out how to get the CI (i feel like one of the existing CI jobs should be able to handle it?) to run the equivalent of |
|
@MoralCode This looks great narrowing Mypy’s scope to the database files is a very practical approach. Using uv run mypy fits nicely into our existing workflow. The main thing now is ensuring this check runs automatically in CI to prevent type regressions in future changes. I think all that’s needed is to add a step in .github/workflows/checks.yml to run Mypy. Once in place, this will work seamlessly, and we can gradually expand the set of type-checked files over time. |
Is this something you want to do/think you would be able to do? |
I think I can try to work on this workflow addition. Would love to get your help in between though. |
|
Sounds good! |
|
I’m happy to build on this PR so everything lands together. But if you’d prefer to merge this first and handle the CI setup separately, that works for me too — totally your call! |
MoralCode
added
ready
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
MoralCode
added
the
ready
You only made 5 commits. I don't think we need to squash those ... but I am kind of a pack rat. |
sgoggins
left a comment
sgoggins
left a comment
There was a problem hiding this comment.
Checking on if Augur's been tested with these changes on your bench. :)
|
|
||
|
|
||
| def extract_needed_issue_message_ref_data(message: dict, issue_id: int, repo_id: int, tool_source: str, tool_version: str, data_source: str) -> List[dict]: | ||
| def extract_needed_issue_message_ref_data(message: dict, issue_id: int, repo_id: int, tool_source: str, tool_version: str, data_source: str) -> dict: |
There was a problem hiding this comment.
Did you run Augur with these changes? On the face of it I am mildly curious if changing from List[dict] to a dict will alter expected return values for other parts of Augur.
There was a problem hiding this comment.
Python typings are just hints, they aren't actually going to change the properties of the data thats returned or cause errors by themselves (unless you like make a syntax error or try and use a type thats not present or something)
|
I can do a quick check tomorrow to make sure augur still boots up and appears to work. All the changes here should be logically identical to how the code was beforehand. This is just setting up to have a type checker be able to validate that the declared types match with how the python objects/data are actually being used |
3 participants
Description
This sets up mypy in a slightly different way to #3280.
to run the type checker, run
uv run mypyThis PR sets up for an eventual fix to #3274
Notes for Reviewers
mostly just making this for reference and to see if it causes the CI jobs to freak out
if this ends up being used, i should probably go back and squash some of the fixup commits i made
Signed commits