Skip to content

lint: run mypy over contrib/devtools #20451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 28, 2020
Merged

lint: run mypy over contrib/devtools #20451

merged 1 commit into from
Dec 28, 2020

Conversation

fanquake
Copy link
Member

wumpus mentioned on IRC that we don't currently run mypy over the contrib/devtools directory, and that it would likely be worthwhile given #20434. This just adds that dir to the linter, as well as some missing annotations to fix existing errors. Note that now we require Python 3.6 we can make use of variable annotations.

master (patched to check contrib devtools):

test/lint/lint-python.sh
contrib/devtools/symbol-check.py:154: error: Incompatible types in assignment (expression has type "List[str]", variable has type "str")
contrib/devtools/circular-dependencies.py:35: error: Need type annotation for 'deps' (hint: "deps: Dict[<type>, <type>] = ...")
contrib/devtools/circular-dependencies.py:67: error: Need type annotation for 'closure' (hint: "closure: Dict[<type>, <type>] = ...")
Found 4 errors in 3 files (checked 187 source files)

I haven't quite gone as far as to add annotations like

CHECKS: Dict[str, List[Tuple[str, Callable[[Any], bool]]]] = {...

to symbol-check.py.

fanquake
fanquake commented Nov 22, 2020
View reviewed changes
@laanwj
Copy link
Member

laanwj commented Nov 22, 2020

Concept ACK

@laanwj
Copy link
Member

laanwj commented Nov 22, 2020 •
edited
Loading

In #20434 (specifically for pixie and symbol check / security check) I've used mypy with --disallow-untyped-calls . I tried --disallow-untyped-defs and --disallow-incomplete-defs as well but that was some more work. probably better to leave it for a later PR.

Better not to overlap work here. I'd prefer if this didn't collide with changes in #20434 (or maybe base it on that).

CHECKS: Dict[str, List[Tuple[str, Callable[[Any], bool]]]] = {... to symbol-check.py.

I didn't figure out that one!

@practicalswift
Copy link
Contributor

practicalswift commented Nov 22, 2020 •
edited
Loading

Concept ACK: fail swift is better than fail slow (at run-time)

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 22, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

This was referenced Nov 22, 2020
Merged
Closed
@fanquake fanquake force-pushed the contrib_devtools_mypy branch 2 times, most recently from 01fda23 to 494a80f Compare November 23, 2020 04:04
@fanquake
Copy link
Member Author

Better not to overlap work here. I'd prefer if this didn't collide with changes in #20434 (or maybe base it on that).

Good call. I've now based this on #20434.

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@fanquake fanquake force-pushed the contrib_devtools_mypy branch from 494a80f to 1ef2138 Compare December 28, 2020 06:26
@fanquake
Copy link
Member Author

This has been rebased post #20434 and should be ready for review.

@laanwj
Copy link
Member

laanwj commented Dec 28, 2020

ACK 1ef2138

@laanwj laanwj merged commit 7f9ae87 into bitcoin:master Dec 28, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 28, 2020
@laanwj
Merge bitcoin#20451: lint: run mypy over contrib/devtools
93ce1b2 
1ef2138 lint: run mypy over contrib/devtools (fanquake)

Pull request description:

  wumpus mentioned on IRC that we don't currently run `mypy` over the `contrib/devtools` directory, and that it would likely be worthwhile given bitcoin#20434. This just adds that dir to the linter, as well as some missing annotations to fix existing errors. Note that now we require Python 3.6 we can make use of variable annotations.

  master (patched to check contrib devtools):
  ```bash
  test/lint/lint-python.sh
  contrib/devtools/symbol-check.py:154: error: Incompatible types in assignment (expression has type "List[str]", variable has type "str")
  contrib/devtools/circular-dependencies.py:35: error: Need type annotation for 'deps' (hint: "deps: Dict[<type>, <type>] = ...")
  contrib/devtools/circular-dependencies.py:67: error: Need type annotation for 'closure' (hint: "closure: Dict[<type>, <type>] = ...")
  Found 4 errors in 3 files (checked 187 source files)
  ```

  I haven't quite gone as far as to add annotations like
  ```python
  CHECKS: Dict[str, List[Tuple[str, Callable[[Any], bool]]]] = {...
  ```
  to `symbol-check.py`.

ACKs for top commit:
  laanwj:
    ACK 1ef2138

Tree-SHA512: a58c2ece588c640289dc1d35dad5b1b8732788272daa0965d6bf44ee8a7f7c8e8585f94d233ac41c84b9ffcfc97841a00fe2c9acba41f58fd164f01de4b6512b
@fanquake fanquake deleted the contrib_devtools_mypy branch February 9, 2021 06:41
@barton2526 barton2526 mentioned this pull request Mar 24, 2022
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

Assignees
No one assigned
Labels
Scripts and tools Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fanquake @laanwj @practicalswift @DrahtBot