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

Types/improve operator node types #2576

Merged

Conversation

mattvague
Copy link
Sponsor Contributor

This PR adds generic op and fn args to OperatorNode type to solve the problems laid out here

Addresses #2575

@mattvague mattvague force-pushed the types/improve-operator-node-types branch from f24cef7 to 8826a66 Compare May 24, 2022 17:18
@mattvague
Copy link
Sponsor Contributor Author

Also just added a generic arg for args so that the following works 😍

image

@mattvague mattvague marked this pull request as ready for review May 24, 2022 18:45
@mattvague
Copy link
Sponsor Contributor Author

I'm wondering now if I should make the op and fn generic args optional as this would be a breaking change otherwise. Thoughts @josdejong?

@josdejong
Copy link
Owner

Thanks, this makes sense.

I'm wondering now if I should make the op and fn generic args optional as this would be a breaking change otherwise. Thoughts @josdejong?

That is a good point. Are there drawbacks to making the generic arguments optional? If so, we can schedule this as a breaking change for v11.

@mattvague
Copy link
Sponsor Contributor Author

That is a good point. Are there drawbacks to making the generic arguments optional? If so, we can schedule this as a breaking change for v11.

Hmm, yeah actually now that I think of it I believe we couldn't do the fancy check that ensures a valid op and fn combo which I feel adds a lot of the value.

If so, we can schedule this as a breaking change for v11.

Should we leave this whole PR until then, or should I create "watered down" version for now where TOp and TFn are just strings?

@mattvague
Copy link
Sponsor Contributor Author

mattvague commented May 27, 2022

Ah, nevermind! I forgot you can just set the default as never. This means that they are optional, but if you do specify them then the combo has to be correct:

image

image

@josdejong
Copy link
Owner

Nice 🤩

@josdejong josdejong merged commit cd33da8 into josdejong:develop May 31, 2022
@josdejong
Copy link
Owner

Published now in v10.6.1

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.

None yet

2 participants