-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds Swerve2/3/4/6ControllerCommand to Commands2 #38
Conversation
This class is provided by the NewCommands VendorDep | ||
""" | ||
|
||
def __init__( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would benefit from the @overload
approach too.
An easy way to see what they should look like is download the 2023 commands whl file, unzip it, and peek at commands2/_impl/__init__.pyi
which has all of the definitions that the prior version defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually had no idea that @overload existed. If you haven't figured it out yet, not a Python programmer :)
Hopefully tomorrow I can work up some updates for the other PRs as well that will hopefully make this much cleaner than me just creating overloads by arbitrarily forcing optional arguments to be required in certain permutations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, overload brings its own share of troubles -- it's just for the type checker, you still have to figure out which set of parameters are incoming.
See
def __init__(self, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed a change to this one implementing overload.
I don't know if this is too much of a breaking change, but I forced kwargs for all the non-positional arguments and then added the overload for the type checker. I added more tests to validate, and it seems to "work".
I don't know enough about Python that by explicitly making the subsystem a Tuple and forcing kwargs, what that does on the other end of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out the type checker was not happy about it in Mypy, though when stepping through with a debugger, both in my force Tuple, and in the status quo (before the Tuple change) at runtime addRequirements receives a tuple[Subsystem] as it's type, not a Subsystem (which is what the type checker in Mypy says is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important to remember that type checking is not enforced at runtime, so running code can sometimes work perfectly fine even if mypy is very angry about it. The primary utility of type hints is twofold: often mypy/pyright is right and type errors indicate that you should fix something (and you can find out before running the code), and it also serves as a useful API documentation to indicate what the function is expecting to receive.
I think we should investigate https://github.com/gabrieldemarmiesse/overtake + beartype to see if we can make this multiple dispatch stuff easier to write.
2d59765
to
3a8432e
Compare
…ptional contructor arguments.
Any idea what is going on here with the tests? I can run them locally in the VS Code test runner, and they also run successfully from the command line:
I tried making a Python 3.8 virtual environment on my apple silicon mac, but for some reason can't install robotpy betas with pip in the 3.8 virtual environment, it can't install robotpy-wpiutil for some reason (can't find the package). In the meantime I'll try to build a VM with an older Ubuntu or something, but it's very confusing why the TypeErrors don't exist in my local test runs but they fail on the Ubuntu machines. |
So, I experimented with that exact fix, but when I did that fix I actually had failures locally both in VS Code's test runner and running python -m pytest. What I pushed originally was in response to local failures...but I guess it's good they work in the CI/CD pipeline. Macos 13, apple silicon, Python 3.11 (installed via Homebrew). Edit to add: Confirmed running an amd64 VM via hypervisor. The behavior of Python 3.11 on Apple Silicon is for some reason opposite of the intel-based systems. Which is suprising. |
The swerve APIs changed in beta 4 (robotpy/mostrobotpy#43), so you might not have upgraded your local robotpy installation. |
6508ccc
to
0efaf1d
Compare
Closing in favor of #45 (which was built on this, so thanks) |
#28