Skip to content
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

Vmux signal types #188

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

daniel-montanari
Copy link
Collaborator

Builds on top of #184 to address #187

Rough draft, there are some nuances between 3.8 and 3.10+ due to reworks of the typing library although none of them are gamechangers.

Can keep adding to this if people like this idea.

@clint-lawrence
Copy link
Collaborator

Obviously we need to get #184 landed, but if this works, we should get this merged too! Then the churn on existing scripts only has to happen once.

For testing, it is hopefully pretty easy to setup some definitions that match the existing "old" style definitions and then compare the resulting _signal_map.

Copy link
Collaborator

@clint-lawrence clint-lawrence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty wary of the details of __class_getitem__ breaking. But this is all pretty non-invasive if we don't actually use it in scripts. So I think it is low risk to just go ahead with this.

Big thing we need is some tests that show a virtual mux defined this way ends up with the same _signal_map as the original method.

src/fixate/_switching.py Outdated Show resolved Hide resolved
src/fixate/_switching.py Outdated Show resolved Hide resolved
src/fixate/examples/jig_driver.py Outdated Show resolved Hide resolved
@jcollins1983
Copy link
Collaborator

jcollins1983 commented Jun 20, 2024

We would need to revisit the definition of name
image

This is a bit hard to process when all we want is the name of the multiplexer
image

We could do this, which covers for both when the subclass has been annotated and not. Don't need the str cast around self.__class__.__name__
image

For the annotated RelayMatrixMux results in
image

And for the non-annotated MuxA
image

@clint-lawrence
Copy link
Collaborator

We could do this, which covers for both when the subclass has been annotated and not. Don't need the str cast around self.__class__.__name__

Pretty sure that big long class name comes from the call to new_class here, so we can at least control what it is at the time it's created. I'm not entirely sure what is a good option though...

Now that @daniel-montanari has done the hard work to make both the sub-class and the type alias version work, we should just decide that you need to subclass and be done with it. Then all the complexity goes away at the expense of a slightly more verbose definition.

@daniel-montanari
Copy link
Collaborator Author

We would need to revisit the definition of name image

This is a bit hard to process when all we want is the name of the multiplexer image

We could do this, which covers for both when the subclass has been annotated and not. Don't need the str cast around self.__class__.__name__ image

For the annotated RelayMatrixMux results in image

And for the non-annotated MuxA image

I've changed the behaviour of this to digest all the type info upfront which then results in a class the same as if you had defined the signals manually.

I still chucked the signal definition to give a differentiation of which mux it should be, but that still results in really long error messages, more to come...

src/fixate/_switching.py Outdated Show resolved Hide resolved
src/fixate/_switching.py Outdated Show resolved Hide resolved
src/fixate/_switching.py Outdated Show resolved Hide resolved
@daniel-montanari
Copy link
Collaborator Author

Given up on support for 3.8.
Push this back until after we switch to 3.12

@daniel-montanari
Copy link
Collaborator Author

I hope the deploy failing is because my code breaks on 3.8 and not actually because it couldn't install Node.js

@jcollins1983
Copy link
Collaborator

This comment is far enough away from the other one that gives it context that I think it warrants a rewording.
image

Maybe # RelayMatrixMux is an example of a Generic class being reused.

@clint-lawrence
Copy link
Collaborator

I hope the deploy failing is because my code breaks on 3.8 and not actually because it couldn't install Node.js

You can drop the other python versions from CI test matrix on this branch if you want.

@daniel-montanari
Copy link
Collaborator Author

I hope the deploy failing is because my code breaks on 3.8 and not actually because it couldn't install Node.js

You can drop the other python versions from CI test matrix on this branch if you want.

Yep was code not working due to python versions not anything to do with the build process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants