Mypy

[daniel-montanari]

No description provided.

[daniel-montanari]

I don't love how I've handled dependencies, possibly another way to go about it is just ignore 3rd party stuff all together, and globally allow for missing imports. This way the only dependency is mypy. I had a look at what we miss out on by not getting type information from other packages, and currently it looks like it actually makes things worse - we get nothing useful out of knowing the types since our own code isn't typed yet, but we still have to deal with suppressing warnings due to runtime import mechanics confusing mypy.

[clint-lawrence]

Me just playing. Didn't think too hard about it, just wanted to see what kind of progress I could make https://github.com/clint-lawrence/Fixate/tree/mypy-experiments

Not free of errors (~10 left), but I did fix a fair few.

[daniel-montanari]

Me just playing. Didn't think too hard about it, just wanted to see what kind of progress I could make https://github.com/clint-lawrence/Fixate/tree/mypy-experiments

Not free of errors (~10 left), but I did fix a fair few.

src\fixate\drivers\lcr\agilent_u1732c.py:95: error: Signature of "frequency" incompatible with supertype "LCR"

• overriding None with int
  src\fixate\drivers\lcr\agilent_u1732c.py:95: error: Signature of "frequency" incompatible with supertype "LCR"
• overriding attribute with property (bug in mypy)
  Current version (0.971) does not allow overriding attributes with properties.
  Fixed here: Allow overriding attribute with a settable property python/mypy#13475 (comment)
  This should hopefully be added to the next release
  Tested using 0.990+dev from master fixes this.

src\fixate\drivers\funcgen\rigol_dg1022.py:399: error: Name "amplitude_ch1" already defined on line 391

• mypy doesn't seem to like if you the property setter is not immediately after the property
  Property setter not accepted if not next to getter python/mypy#1465

src\fixate\drivers\funcgen\rigol_dg1022.py:449: error: "Callable[[FuncGen], Any]" has no attribute "setter"

• While it isn't great that mypy doesn't support this, I think defining properties on an abstract parent is odd design, probably should change them to normal attributes, then the overriding class can choose to implement them as properties or not
  Can't declare a property setter on a child class without declaring getter there as well python/mypy#4644

src\fixate\drivers\pps_init.py:22: error: Incompatible types in assignment (expression has type "BK178X", variable has type "SPD3303X")
src\fixate\drivers\pps_init_.py:23: error: "SPD3303X" has no attribute "baud_rate"_

• looks like the logic in this function is confusing mypy into thinking that it is reassigning driver due to the shadowed variable name

[daniel-montanari]

if struct.calcsize("P") == 8:  # 64 bit
    FT_HANDLE = ctypes.c_ulonglong
else:
    FT_HANDLE = ctypes.c_ulong

Mypy forces you to either rewrite this as a conditional, or explicitly annotate the type as a Union yourself :(

[clint-lawrence]

if struct.calcsize("P") == 8:  # 64 bit
    FT_HANDLE = ctypes.c_ulonglong
else:
    FT_HANDLE = ctypes.c_ulong

Mypy forces you to either rewrite this as a conditional, or explicitly annotate the type as a Union yourself :(

FH_HANDLE = ctypes.c_void_p should fix that and is a more appropriate type. It is not a Union!

[clint-lawrence]

Overall, I'm happy to merge. I think you should get buy in from everyone first though. (@John2202W , @jcollins1983 , @christopherwallis , @Jasper-Harvey0 )

The main thing I'm wary off is not having a clear goal stated. For example, that goal could be

• Have all interfaces intended for use in scripts annotated
• Have the entire codebase annotated

The first should probably be the priority. (e.g. we spend more time writing scripts than modifying fixate)

And I've already said this before, but not sure I've committed it in writing - we almost certainly shouldn't try annotating the code as is. There is all kinds of busted stuff that most likely needs to be refactored and/or redesigned before annotation will be useful.

[daniel-montanari]

I've updated the exclude list since files have changed in the last few months.
I've also added all excluded files to the silent imports list. This means that importing an excluding module won't create errors due to mypy analysing the excluded module when it is imported by another module.
I've also updated mypy to the latest version.

I haven't gone through and changed any of the config rules to non-default. We can merge this first and then update the config later.

[jcollins1983]

Overall, I'm happy to merge. I think you should get buy in from everyone first though. (@John2202W , @jcollins1983 , @christopherwallis , @Jasper-Harvey0 )

The main thing I'm wary off is not having a clear goal stated. For example, that goal could be

• Have all interfaces intended for use in scripts annotated
• Have the entire codebase annotated

The first should probably be the priority. (e.g. we spend more time writing scripts than modifying fixate)

And I've already said this before, but not sure I've committed it in writing - we almost certainly shouldn't try annotating the code as is. There is all kinds of busted stuff that most likely needs to be refactored and/or redesigned before annotation will be useful.

I'm generally in favour of this, particularly if it means you don't have to go digging into functions that are supposed to be providing a layer of abstraction to work out what they actually need to be provided, so I agree with Clint that the priority should be to annotate all script facing interfaces.

[John2202W]

I'm in favour of gradual introduction of type hinting - haven't played much with mypy but appears a good route.

[christopherwallis]

I'm definitely supportive of this too. Should (eventually) make a lot of things easier, especially the changes to script interfaces. I haven't tested anything and I'm not completely up to date with fixate or mypy but everything looks good based on my read through of the pull request.
The comment about caching with @daniel-montanari's last change might need updating though.