Add static type checking with MyPy

[cthoyt]

Blocked by #146

This PR does the following:

• Adds mypy environment to tox.ini
• Adds relevant configuration to pyproject.toml to exclude auto-generated modules
• Adds mypy testing to CI configuration
• Fixes all type annotations
• Fix remaining tests

[cthoyt]

@hrshdhgd do you want to help me with this one? We should definitely merge #142 first, but then this might be a good experience for you as well

[hrshdhgd]

On it!

[hrshdhgd]

I'm assuming you've already taken care of the 1st three bullets?

[cthoyt]

Yes, the bullets that I checked are already done in this PR

[hrshdhgd]

Some changes that satisfy Mypy result in errors in pytest. Is that expected?

[cthoyt]

Some changes that satisfy Mypy result in errors in pytest. Is that expected?

Not sure what you mean - let me know if you see this being reproduced in the CI tests. For the most part, changing type annotations doesn't change executed code (except in the case of forward references... you can google that if you do end up with an issue like this)

[hrshdhgd]

Some changes that satisfy Mypy result in errors in pytest. Is that expected?

Not sure what you mean - let me know if you see this being reproduced in the CI tests. For the most part, changing type annotations doesn't change executed code (except in the case of forward references... you can google that if you do end up with an issue like this)

sssom/util.py:67: error: Incompatible types in assignment (expression has type "None", variable has type "Dict[str, Any]")

So I fix it by changing
prefixmap: Dict[str, Any] = None # maps CURIE prefixes to URI bases
to
prefixmap: Dict[str, Any] = {} # maps CURIE prefixes to URI bases

fixes Mypy but when I run pytest, this fails.

____________________________________________________________________ ERROR collecting tests/test_cli.py _____________________________________________________________________
tests/test_cli.py:6: in <module>
    from sssom.cli import (
sssom/__init__.py:3: in <module>
    from .util import (  # noqa:401
sssom/util.py:61: in <module>
    class MappingSetDataFrame:
/opt/anaconda3/envs/sssom-new-linkml/lib/python3.8/dataclasses.py:1019: in dataclass
    return wrap(cls)
/opt/anaconda3/envs/sssom-new-linkml/lib/python3.8/dataclasses.py:1011: in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash, frozen)
/opt/anaconda3/envs/sssom-new-linkml/lib/python3.8/dataclasses.py:861: in _process_class
    cls_fields = [_get_field(cls, name, type)
/opt/anaconda3/envs/sssom-new-linkml/lib/python3.8/dataclasses.py:861: in <listcomp>
    cls_fields = [_get_field(cls, name, type)
/opt/anaconda3/envs/sssom-new-linkml/lib/python3.8/dataclasses.py:745: in _get_field
    raise ValueError(f'mutable default {type(f.default)} for field '
E   ValueError: mutable default <class 'dict'> for field prefixmap is not allowed: use default_factory

[hrshdhgd]

fixes Mypy but when I run pytest, this fails.

The fix seems to be prefixmap: Dict[str, Any] = field(default_factory=dict) # maps CURIE prefixes to URI bases but I have never seen something like this before. Thoughts?

Source

[cthoyt]

fixes Mypy but when I run pytest, this fails.

The fix seems to be prefixmap: Dict[str, Any] = field(default_factory=dict) # maps CURIE prefixes to URI bases but I have never seen something like this before. Thoughts?

Source

Yes that's the fix! Welcome to the wild world of dataclasses. But when you add one of these factories, you should also ask yourself if a developer should ever actually be able to create an instance of this class without an predefined prefix map. There are a lot of instances where it seemed to me that this hadn't been considered

[cthoyt]

@hrshdhgd please stop making updates here until you review #145. This will make more work to merge after if you keep going

[cthoyt]

@hrshdhgd thanks for being patient - I merged the other PR then updated into this one, so we can continue here

[matentzn]

Hey both, this PR lets my heart race in happiness - please let me know if you need me to take over and finish it - we should merge asap and make a new release, and hopefully than this was the last of the massive refactor PRs for a while :) thank you both.

Just one thing: lets please strictly adhere to mypy here and not address any other issues. If you find other stuff, make issue for them.

[cthoyt]

@matentzn @hrshdhgd I've finished updating all of the type checking (only in a few places using ignores, such as the crazy typing in the autogenerated code). There were a lot of places where the handling of falsy prefix maps (e.g., None or empty) were not explicit. This made it really hard to figure out what the business logic should be. Because I've done what it took to make the program make sense, there are now a few failing tests, so I'd suggest Nico goes through the deep set of functions handling these prefix maps and figures out which business logic should be the right one.

I know it was your goal to do minimum code changes, but type checking exposes logical errors that fully call for refactoring and bug fixing, so we're just lucky it wasn't more

[hrshdhgd]

Tests Pass!!

[cthoyt]

Everyone can take a look tomorrow to see if any of the changes we made weren't covered by tests. Otherwise, let's get this merged and move on with more fun refactoring in a future PR!

[cthoyt]

Special thanks to @hrshdhgd who got up to speed on this type checking stuff super fast!!

[matentzn]

This is a huge applause to both of you. I am impressed - some of the python is a good deal beyond my current ability and just reading this PR has thought me a great deal.

If we could make sure that make test also runs the mypy, that'd be great.

[cthoyt]

If we could make sure that make test also runs the mypy, that'd be great.

In 778bfca, I added mypy to the default list of environments that's run with tox. Since the tox command is run in the test goal, this does the trick! I also added a mypy-specific goal in f75b724

[matentzn]

Thank you so much @cthoyt - If one of you could resolve all the open conversations I think this can be merged!