falconry / python-mimeparse

Basic functions for handling Internet media types in Python.
MIT License
19 stars 11 forks source link

feat(typing): add type hints #52

Closed rafalkrupinski closed 3 weeks ago

rafalkrupinski commented 3 weeks ago

Didn't use TypeAlias bc it's python>=3.10 so it would require typing_extensions (can be added if necessary).

quality_and_fitness_parsed declares rtype: (float,int) but the second value adds value of q, which may be float.

rafalkrupinski commented 3 weeks ago

Wow, we were not really planning much development here

I wanted to do #50 and it always helps to know what's being passed around.

shouldn't we include an empty py.typed file

I'll take a look what to do about py.typed file in this case. Might need to move code to mimeparse/__init__.py, like you've said.

Furthermore, running mypy tells me there is room for improvement

I did check with mypy, and the return type hints are correct, only at some point inside functions the variables don't match them.

For example a weight is initialized with 0 and then at some point a float is added, or dict value is set to None and then overridden with some other value 🤷.

I wanted to keep the change minimal so I've decided to keep it. I'm happy to change it if you're planning to add pre-commit with mypy ;)

vytas7 commented 3 weeks ago

Hm, I'm no expert at typing, but I think it would be good to check Mypy's output and see if we can fix the code inside the functions too. Otherwise, if we just care about the user experience for using mimeparse, maybe we should simply a stubs instead?

What do you think @CaselIT?

rafalkrupinski commented 3 weeks ago

Wow, we were not really planning much development here

Do you consider this package as internal to falconry? Let me know please, if so, I can always fork, so no biggie.

edit: I've changed the sentence, I wasn't too sure about how others might take it.

vytas7 commented 3 weeks ago

Wow, we were not really planning much development here

Do you consider this package as internal to falconry? No problem, I can always fork.

No, not internal at all. We are actually thinking even to reimplement parts of it and unvendor it from Falcon completely.

By writing that, I just meant that this package didn't attract many contributions in the 8 years since the last release :slightly_smiling_face: (We took over about 3 years ago, since we are using it, even if vendored inside, as it was getting abandoned.)

rafalkrupinski commented 3 weeks ago
$ mypy mimeparse
Success: no issues found in 1 source file
rafalkrupinski commented 3 weeks ago

Yeah, I see that. I thought the changes wouldn't cause any issue, didn't think to run the tests :/

rafalkrupinski commented 3 weeks ago

I use it in my project, and I really like having less code to maintain, so...