Fix most mypy errors. #403

tristanlatr · 2021-06-16T18:15:48Z

No description provided.

codecov · 2021-06-16T18:16:59Z

Codecov Report

Merging #403 (c96d8df) into master (566286b) will decrease coverage by 0.04%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
- Coverage   87.30%   87.26%   -0.05%     
==========================================
  Files          22       22              
  Lines        4262     4264       +2     
  Branches      839      840       +1     
==========================================
  Hits         3721     3721              
- Misses        350      351       +1     
- Partials      191      192       +1

Impacted Files	Coverage Δ
pydoctor/model.py	`91.17% <0.00%> (-0.36%)`	⬇️
pydoctor/epydoc/markup/restructuredtext.py	`85.11% <100.00%> (ø)`
pydoctor/templatewriter/__init__.py	`90.37% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 566286b...c96d8df. Read the comment docs.

tristanlatr · 2021-06-19T21:27:24Z

Opinions on the two issues @adiroiban @mthuurne ?

pydoctor/epydoc/markup/restructuredtext.py:55: error: Module "docutils.parsers.rst" has no attribute "directives"; maybe "Directive"?  [attr-defined]
pydoctor/epydoc/markup/restructuredtext.py:395: error: Argument 1 to "OptionParser" has incompatible type "List[Any]"; expected "Optional[str]"  [arg-type]

mthuurne · 2021-06-20T20:40:41Z

Sorry for my slow responses; it's been a busy week and we had a bit of a heatwave here. I'll have a look now.

mthuurne · 2021-06-20T20:59:24Z

pydoctor/epydoc/markup/restructuredtext.py:55: error: Module "docutils.parsers.rst" has no attribute "directives"; maybe "Directive"?  [attr-defined]
pydoctor/epydoc/markup/restructuredtext.py:395: error: Argument 1 to "OptionParser" has incompatible type "List[Any]"; expected "Optional[str]"  [arg-type]

The first error is weird: I can reproduce it under tox, but in my manually maintained venv it doesn't occur, even if I'm using the same version of mypy, docutils, and types-docutils.

The second error I can reproduce in both setups.

tristanlatr · 2021-06-20T22:39:31Z

Ok. So let’s ignore them ?

mthuurne · 2021-06-20T22:52:27Z

Ok. So let’s ignore them ?

I want to dig a bit deeper first.

mthuurne · 2021-06-20T23:13:55Z

The problem with OptionParser seems to be that docutils.frontend.OptionParser inherits from optparse.OptionParser, but has a different __init__() signature, which is missing from the type stubs. Therefore this one can be ignored indeed, with a link to python/typeshed#5667 so we know what is happening and when we can remove the suppression.

mthuurne · 2021-06-20T23:27:30Z

docutils.parsers.rst.directives is a module, which is present in docutils itself but missing from typing-docutils. So this is another case of the docutils stubs being incomplete.

I still don't understand why I don't get the error in my dev venv: I thought the reason might be because I have docutils installed, but the mypy tox environment also installs docutils since it's a dependency of sphinx, which we depend on since they ship type annotations in their main distribution itself.

So ignoring the error would be the correct short-term solution here as well. You could link to python/typeshed#1269 as documentation of the problem.

mthuurne · 2021-06-21T00:11:11Z

pydoctor/model.py

@@ -808,6 +808,7 @@ def introspectModule(self,
            module_full_name = f'{package.fullName()}.{module_name}'

        spec = importlib.util.spec_from_file_location(module_full_name, path)
+        assert spec is not None, f"Cannot find spec for module {module_full_name} at {path}"


We should only use assert if we can we guarantee that the condition is always true. If we're not that sure, it would be better to raise an exception instead.

I think there is at least one unlikely but possible scenario in which this condition is not true: if a file is deleted or moved between the moment its name is discovered and the moment it is introspected.

A RuntimeError is OK ?

pydoctor/templatewriter/__init__.py

…peshed#1269

I don't think we should fix them all in this PR.

…tils related typing.

tristanlatr · 2021-06-22T02:02:23Z

You could link to python/typeshed#1269 as documentation of the problem.

I've tried using the docutils-stubs as mentioned in this ticket but it's becoming a real headache, more than 50 errors that require a lot of additional checks in the restructuredtext module. I don't want to fix all mypy errors honestly. I will revert back to use types-docutils for the principal mypy env and create a separate one that uses docutils-stubs so we can look at it later.

Also the HTMLTranslator class has been moved to pydoctor.node2stan._PydoctorHTMLTranslator in #386

…ubs." This reverts commit bc3f2ac.

glyph

🚢

glyph · 2021-06-22T05:38:03Z

tox.ini


 commands =
    mypy                                       \
        --cache-dir="{toxworkdir}/mypy_cache"  \
        {tty:--pretty:}                        \
        {posargs:pydoctor docs/epytext_demo}

+[testenv:mypy-docutils-stubs]
+description = run mypy with docutils-stubs (does not pass for now)
+; See: https://github.com/python/typeshed/issues/1269


Do we want an issue for this repo as well, to track turning it on in CI when this is done?

glyph · 2021-06-22T05:38:38Z

pydoctor/model.py

@@ -808,6 +808,8 @@ def introspectModule(self,
            module_full_name = f'{package.fullName()}.{module_name}'

        spec = importlib.util.spec_from_file_location(module_full_name, path)
+        if spec is None: 
+            raise RuntimeError(f"Cannot find spec for module {module_full_name} at {path}")


Can you do a test for this apparently untested case in a follow-up?

Like @mthuurne was saying, this error is raised when a file is deleted or moved between the moment its name is discovered and the moment it is introspected. So honestly I don't know how to test such a case.

This is why FilePath narrows down its interface to a single object which returns other objects, so you can pass a parameter along to the system under test. In the absence of such compositional design, the way to test this is to mock out spec_from_file_location with patch in a test.

tristanlatr · 2021-06-22T12:34:31Z

Thank @glyph for your review.

I'll let @mthuurne merge this PR.

@glyph, If you want to help out reviewing more PRs, that would be great.
There is #386 waiting for a review ;)

glyph · 2021-06-22T18:07:50Z

@glyph, If you want to help out reviewing more PRs, that would be great.

I'd love to, but WYSIWYG in this case — I already have a recurring task in my personal task management system to review open source PRs as frequently as I can, and … this is about it ;)

tristanlatr · 2021-06-30T16:15:41Z

@mthuurne Can I merge that ? Thanks

tristanlatr · 2021-07-07T14:00:25Z

@mthuurne I will merge this PR now since it's blocking the CI from being green on all other branches.
If there is anything you'd like to be modified, please speak up and I'll submit a patch.

Fix most mypy errors.

f6f871b

tristanlatr requested a review from mthuurne June 16, 2021 18:16

tristanlatr requested a review from adiroiban June 16, 2021 20:42

mthuurne mentioned this pull request Jun 20, 2021

Remove overloading of epytext parse() function #407

Closed

mthuurne requested changes Jun 21, 2021

View reviewed changes

tristanlatr added 6 commits June 21, 2021 20:12

Raises RuntimeError instead of using assert.

87c210e

Use docutils-stubs instead of types-docutils to workarround python/ty…

a8059b4

…peshed#1269

Fix a few of the mypy errors detected after using docutils-stubs.

bc3f2ac

I don't think we should fix them all in this PR.

Revert part of the changes to use Node as annotation, not Element.

7978be2

Add a TODO

71581a6

Add a separate mypy env 'mypy-docutils-stubs' to work further on docu…

61cd3e8

…tils related typing.

tristanlatr added 2 commits June 21, 2021 22:08

Revert "Fix a few of the mypy errors detected after using docutils-st…

386cdeb

…ubs." This reverts commit bc3f2ac.

Add type ignore comments

13d21cb

tristanlatr mentioned this pull request Jun 22, 2021

Fix the mypy errors raised by tox -e mypy-docutils-stubs #409

Open

tristanlatr requested a review from mthuurne June 22, 2021 02:19

Add comments

c96d8df

glyph approved these changes Jun 22, 2021

View reviewed changes

tristanlatr mentioned this pull request Jun 23, 2021

Use to ConfigArgparse to parse arguments and load config file #411

Closed

tristanlatr merged commit 86cd1f5 into twisted:master Jul 7, 2021

Fix most mypy errors. #403

Fix most mypy errors. #403

Uh oh!

Conversation

tristanlatr commented Jun 16, 2021

Uh oh!

codecov bot commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tristanlatr commented Jun 19, 2021

Uh oh!

mthuurne commented Jun 20, 2021

Uh oh!

mthuurne commented Jun 20, 2021

Uh oh!

tristanlatr commented Jun 20, 2021

Uh oh!

mthuurne commented Jun 20, 2021

Uh oh!

mthuurne commented Jun 20, 2021

Uh oh!

mthuurne commented Jun 20, 2021

Uh oh!

mthuurne Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

tristanlatr Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tristanlatr commented Jun 22, 2021

Uh oh!

glyph left a comment

Choose a reason for hiding this comment

Uh oh!

glyph Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

tristanlatr Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

glyph Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

tristanlatr Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

glyph Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

tristanlatr commented Jun 22, 2021

Uh oh!

glyph commented Jun 22, 2021

Uh oh!

tristanlatr commented Jun 30, 2021

Uh oh!

tristanlatr commented Jul 7, 2021

Uh oh!

Uh oh!

codecov bot commented Jun 16, 2021 •

edited

Loading