Add type hinting #470
Add type hinting #470
Conversation
There was a problem hiding this comment.
Overall, thank you for putting in the effort to do this. It's big but most of the changes are pretty uniform. I'd extract out the init stuff to a separate PR and see if that fixes the tests. I sorta feel the enum conversions should be separate too... but I dont mind it quite so much.
I think the next step is adding something that ensures any code in the repo that's added gets type hints, but we can save that for another day.
|
Is type hinting the |
|
Given that we already import everything anyways, I don't see any harm in it. |
5231139 to
9f824ed
Compare
|
If someone could help me with these circular imports that'd be much appreciated |
|
Either take a read of PEP 484, or just don't have circular imports in the first place. |
1504598 to
a4e52f9
Compare
a987396 to
ecdef81
Compare
|
You rebased the wrong way around. |
|
I didn't pulll...idk how that happened |
|
@virtuald Are you fine with making the type signatures in SmartDashboard API? |
|
I think as long as we're not verifying the signatures automatically via mypy, that getting close enough is good enough. We can fix things piecemeal after this PR. |
|
To me, "close enough" would be saying that we're only guaranteeing that putting a sequence on NetworkTables will work. Iterable seems pedantic and potentially confusing. Do we want to make the guarantee that iterables can always be put onto NetworkTables? |
|
@auscompgeek when it puts things in networktables, it iterates over them and turns it into a tuple. The final product is a tuple, but any iterable will be able to be parsed |
|
Yes, but a sequence is also iterable. We also haven't fully defined the types throughout all of pynetworktables (a bunch of which the documented types are wrong anyway). My question still stands. |
|
The implementation is at https://github.com/robotpy/pynetworktables/blob/59b5d1328274e78393202fe2a7e09845f5ff63a5/ntcore/value.py#L57 , using an iterable seems best? |
wpilib/wpilib/sendablechooser.py
Outdated
| @@ -5,8 +5,12 @@ | |||
| # must be accompanied by the FIRST BSD license file in the root directory of | |||
| # the project. | |||
| # ---------------------------------------------------------------------------- | |||
| from typing import TypeVar | |||
|
|
|||
| V = TypeVar("V") | |||
There was a problem hiding this comment.
This type variable isn't particularly useful if it doesn't tie any types together.
If this is intended to tie together the argument and return types for SendableChooser's methods, you might also want to make this type variable covariant.
(i.e. if I have a SendableChooser of SpeedControllers, I should be able to put in a Spark, get it back, and treat it as merely a SpeedController.)
There was a problem hiding this comment.
Wouldn't a spark fall under SpeedController since it's a subclass? I add a SpeedController object (which happens to be a spark), and I get back a speedcontroller object (that is also of type spark)
There was a problem hiding this comment.
We appear to be in violent agreement with each other.
There was a problem hiding this comment.
Oh I didn't realize python had a special marking for this type of behavior. All these small quirks have made me wonder if I should've read through pep484 before implementing it...🤔
|
Any other nitpicks, make a new PR. |
No description provided.