-
Notifications
You must be signed in to change notification settings - Fork 16
Merge NamedTuple transforms by merging the underlying NamedTuples #150
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
Conversation
devmotion
left a comment
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.
Generally it would be good to ensure it's inferred.
|
Yes, please mention it in the docs, eg in this sectio. |
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
|
I've added a little note to the docs which suggests the way I am using this; if it should be broadened or not have the link to |
|
The note in the docs is fine, the documentation is probably due a rewrite one of these days so I will mention this. |
|
Is there anything else this needs before being merge-able (haha)? |
|
Yes, can you please add a trivial test set with collisions (overlapping names)? It is fine to rely on After that, I will merge immediately, I think you address the concerns of @devmotion. Thanks for all the great work! |
|
I've added a test for name collisions (and reworded the docs to demonstrate the same thing). Thanks! |
This may not be strictly necessary, but I have found it convenient to be able to merge multiple NamedTuple transformations by merging the underlying NamedTuples. Previously I did this by relying on the internal fields, but that approach broke (which is totally reasonable, it was an implementation detail).
Should this be a documented feature, in addition to tested?