Define typings for simdjson

TkTech / pysimdjson

Python bindings for the simdjson project.

https://pysimdjson.tkte.ch

Other

643 stars 54 forks source link

Define typings for simdjson #76

Closed kornicameister closed 3 years ago

kornicameister commented 3 years ago

Closes: #75

kornicameister commented 3 years ago

@TkTech that is very basic idea for the 1st round of discussions :))

kornicameister commented 3 years ago

@TkTech consider this patch a WIP. I need to deal with some issues around __getitem__ in Array. I will push the changes as soon as I have time to do them.

TkTech commented 3 years ago

Looking good, added a few comments, but I'm not super familiar with mypy.

kornicameister commented 3 years ago

@TkTech that's why I am constantly checking how those typings perform against my own codebase. One thing that grabbed my attention was that in fact we can define neither Object nor Array as generic type. Such definition makes mypy complain that we either of those misses type. I don't think that's the case with pysimdjson to say Object[str] or Array[Union[str, int]], right?

kornicameister commented 3 years ago

FYI: a little of typings in action: xxx

kornicameister commented 3 years ago

@TkTech out of pure curiosity? Should I expect another review round, merging this one up or will it wait for you being ready to proceed with cythonize branch? I am fine either way :D just curious :)

TkTech commented 3 years ago

Just waiting for v4 (the cythonize branch) to get finished, API isn't 100% stable yet but it's pretty close.

How are you testing these typings? It would be nice to have mypy run them in the test workflow.

kornicameister commented 3 years ago

Actually, that is a very good point. I will write some of those. I haven't checked your framework of choice but if that is pytest it shouldn't be big of a deal.

kornicameister commented 3 years ago

@TkTech tests added, your GHA config does not pick my fork so there's no confirmation visible. Let me paste a screen in that case.

TkTech commented 3 years ago

Awesome! Sorry there hasn't been movement on this, just been a bit too busy to finish up v4.

TkTech commented 3 years ago

This is included in 4.0.0, with a couple corrections. Unfortunately I removed the tests, they were far more of a pain (why, why does it include line numbers in the output!) then they're worth at the moment. I plan to revisit the testing for this.

kornicameister commented 3 years ago

@TkTech as far as I remember there is/was a possibility to turn off line numbers from mypy output. If you will have any issues you can always drop me an issue on repository. I might be able to take a look.