mypy error: Skipping analyzing "crc": [...] missing library stubs or py.typed marker

[gertvdijk]

Thanks for having type annotation in your package! However, I can't use it with type checkers in my own code, because crc is missing a py.typed marker file. Please see PEP 561 for more information.

This leads to errors in downstream projects using type checkers like mypy:

error: Skipping analyzing "crc": module is installed, but missing library stubs or py.typed marker  [import]
note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

Looking at your source code package...:

$ tar -tf crc-4.3.0.tar.gz 
crc-4.3.0/LICENSE.txt
crc-4.3.0/README.md
crc-4.3.0/crc.py
crc-4.3.0/pyproject.toml
crc-4.3.0/setup.py
crc-4.3.0/PKG-INFO

... I understand your current distribution is single-file/module-only, but it seems that PEP requires a refactor into a package structure.

This PEP does not support distributing typing information as part of module-only distributions or single-file modules within namespace packages.

The single-file module should be refactored into a package and indicate that the package supports typing as described above.

Let me know if you need some help with that; I'm happy to provide a PR.

[Nicoretti]

Hi @gertvdijk,

first of all, thanks for creating the issue, it's a perfect summary of the problem. 👍

The TL;DR from my point of view is:

1. I intentionally designed the library to be a single module file.
2. After adding the type information etc. I realized that the typed marker currently is not supported for single file module distributions.
3. I decided against the restructuring because having a single module file was one of my major design decisions back in the day.

That said, since then quite some time passed and I had some time to think about this issue. Actually I already where considering implementing it, but did not find the time yet. Therefore if you want to provide a PR, I would be more than happy to integrate it.

best
Nico

[gertvdijk]

Cool. Two questions for you.

---

On the restructuring, I think we have two options:

1. Move crc.py to src/crc/__init__.py as-is.
2. Move crc.py to src/crc/crc.py unchanged, add an src/crc/__init__.py with an __all__ that only exposes the (common) public API objects. The benefit here is that it allows to have a cleaner namespace/tab-completion. I will check what's mentioned in docs to see what to include in __all__. If users are missing something they could always import the module of the package directly (import crc.crc) and go from there.

Both options keep it single-file in some definitions of it. Users who depend on downloading a crc.py and vendor it in will have a little benefit in option 2 given the filename. OTOH, a curl --output crc.py https://github[...]/src/crc/__init__.py isn't that much of work to adjust for.

Any preference?

Anyway, I have zero experience with Poetry when it comes to packaging, so it's a little less straightforward for me, but I think I will manage.

---

I have some thoughts about some of the type annotations too, might as well address those. Spotted some unnecessary type: ignores, and would like to see mypy pass in strict mode while at it. Took me a few minutes to fix that.

Since the module requires Python 3.8, I am planning to refactor it into using built-in type annotations (available since 3.7 with from __future__ import annotations, postpones evaluation on < 3.9). PEP 563, dropping most of the from typing import [...] ones. Seems like a more modern approach.

WIP here with some other edits too - view commit by commit, WDYT?

---

[Nicoretti]

@gertvdijk I am currently on a trip, it's likely I won't be able to give you feedback/responses until next week.
Thanks for contributing, I really appreciate it 👍 .

[Nicoretti]

Hi @gertvdijk,

I personally would tend to go for option 2.

   src
    └── crc
        ├── __init__.py
        └── _crc.py

Also marking the crc module as non public _crc.py

ℹ️ If you need any help with poetry let me know!

Regarding

I have some thoughts about some of the type annotations too, might as well address those. Spotted some unnecessary type: ignores, and would like to see mypy pass in strict mode while at it. Took me a few minutes to fix that...

Sure go for it 👍 .

Regarding the change-set (WIP)

Looks good to me. Only small nit pick I have, is that I would prefer naming: DataInputType to something shorter like InputType or so. Still I am open to get feedback why DataInputType would be the better name 😉 .

[gertvdijk]

Hi @Nicoretti! Thanks for the answers. I have spent some time on moving things in the right direction, I think; see the updated branch.

Open points:

• The type alias InputType looks good in code, but not good in docs; the alias isn't revealed in mkdocstrings, apparently. I'll have look later, or else drop it for now. Perhaps I introduced more changes like that which I am unaware of.
• I am unsure about whether I really cover everything for the change; I don't know how to run the GH actions locally for example. I've tested it locally by creating a local package with poetry build, installed it in my downstream package's venv and it works as intended removing the mypy suppression as far as I can see. 🎉
• The linters/checks return a quite some errors already before this change. I suspect you just disregarded most of them or something? (I am more used to running it all and starting at 0 errors when on a clean stable branch.)
• I wanted to import from the 'public' side of things in the test as much as possible, but somehow the isort configuration changes that to the private module import. 🤷🏼‍♂️

(If you're interested, I could file a change to drop all of the linters - except Black - and replace them with Ruff.)

Anyway, please have a look and let me know what you think. 😃

[Nicoretti]

Hi @gertvdijk,

First up: imho looks great 👍 .

Regarding Your Open Points

1. InputType

Some manually added documentation could close the gap for now too.
In the end it is just markdown files.

2. "Is Everything Covered"

Overall it looks good. Tests should cover the "important" part (API and correctness).
If not time, CI and users will tell anyway 😉.

3. Starting at 0 warnings

Not much to say here, I agree with you!

How it came to be:

1. Well initially the module was written to get something else done, which wasn't meant to be a long term solution. That said, a long time ago it got thrown away.
   🗒️ Side Note: That's also where having it as a single file requirement stems from.
2. Thinking having a pure python crc implementation is useful caused me to keep that piece alive
3. Over time I improved and extended the module
4. At some point introduced a linter
5. Never fixed all warnings, due to the lack of time and priorities

4. Isort

Interesting, hadn't had that issue so far. It's maybe not ideal but would be ok from my side.

5. Ruff

I am pretty much in favor of Ruff for new projects.
For legacy projects (!= 0 warnings) I think pylint has the advantage, that by making use of the
score one can easily improve the code base without the need of suppressions or other "tricks".

Having that said, if you want to introduce Ruff into the project, it would be very welcome!
We maybe even could get rid of black (see ruff formatter).

🗒️ Note: I would appreciate the if you would create as a separate PR for that change.

💡 Info:

I would mark the PR(s) with the hacktoberfest marker, if that make sense for you.

[gertvdijk]

Alright, cool, I've created a PR with some more changes to docs and things like a __main__.py that was missing to not break a python -m crc use case.

Feel free to add a hacktoberfest marker if you want (I have no idea how to do that anyway 😄).

LMK what you think; I may have missed things, I'm very much open for remarks/review comments.

[Nicoretti]

@gertvdijk

LMK what you think; I may have missed things, I'm very much open for remarks/review comments.

Honestly very nice and clean PR ❤️ , I even learned a few things I haven't been aware of yet 😅.
So thanks a lot! Will come back to it tomorrow or the next days and merge it once the checks have been run.