Skip to content

MyPy Typechecking #19

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.

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

Closed
0az opened this issue May 18, 2020 · 6 comments · Fixed by #64
Closed

MyPy Typechecking #19

0az opened this issue May 18, 2020 · 6 comments · Fixed by #64
Labels
enhancement New feature or request

Comments

@0az
Copy link

0az commented May 18, 2020

As part of getting to learning how to hack on the package, I've started running MyPy for a "guided tour" of the API.

Do you mind if I send in one or more PRs to fix/silence some of the warnings?

@chrisjsewell
Copy link
Member

Yeh go for it 👍

@chrisjsewell chrisjsewell added the enhancement New feature or request label May 18, 2020
@0az
Copy link
Author

0az commented May 18, 2020

Something that I noticed when walking through the code:

There's a lot of untyped dicts and AttrDicts that probably can get replaced with dataclasses/attrs/namedtuple/etc.

Example:

def postProcess(state: StateInline):
    """Walk through delimiter list and replace text tokens with tags."""
    tokens_meta = state.tokens_meta
    maximum = len(state.tokens_meta)
    _postProcess(state, state.delimiters)

    curr = 0
    while curr < maximum:
        try:
            tokens_meta[curr]
        except IndexError:
            pass
        else:
            if tokens_meta[curr] and "delimiters" in tokens_meta[curr]:
                _postProcess(state, tokens_meta[curr]["delimiters"])
        curr += 1

tokens_meta can probably get swapped over to a TypedDict, or something like:

@dataclass
class TokenMetadata:
	delimiters: List[Delimiter]

The downside of TypedDict is that it's a 3.8+ feature, and some functionality requires typing_extensions until 3.9. One workaround could be using __getitem__/etc.

This is out of scope for what I intend, though.

@0az
Copy link
Author

0az commented May 20, 2020

@chrisjsewell Which test failures should I expect?

@chrisjsewell
Copy link
Member

Which test failures should I expect?

None?
I'm not sure if I understand what you, all the tests currently: https://github.com/executablebooks/markdown-it-py/actions/runs/105415163
Why would you expect some to fail?

FYI in-terma of mypy, you could just try add the pre-commit hook to the pre-commit config file, then fix the code accordingly: https://github.com/pre-commit/mirrors-mypy

@0az
Copy link
Author

0az commented May 21, 2020

Ah, sorry. I got a bug I introduced locally mixed up with some of the skipped tests.

Right now, there's around ~70 standing errors that I still need to fix, after grabbing the lowest hanging fruit, so I'll need to figure something out before adding a pre-commit hook.

@chrisjsewell
Copy link
Member

Yeh no worries. The only thing to bear in mind is that we "shouldn't" (without very good reason) change the code in any way that deviates from the source markdown-it API.

Also I am definitely +1 for TypedDict, given that I am a big fan of TypeScript over JavaScript (essentially where this idea comes from), but yeh unfortunately it's a bit too early to be restricting the code to python 3.8+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants