Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Union matches: select type by number of matching fields #264

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

m-aciek
Copy link
Contributor

@m-aciek m-aciek commented Oct 22, 2024

Closes #263.

Copy link

@airunderscoreland airunderscoreland left a comment

Choose a reason for hiding this comment

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

This looks like it safely solves the issue

@m-aciek
Copy link
Contributor Author

m-aciek commented Oct 23, 2024

There was an unintended bug in my code. I fixed it with 39d01b7.

Python default sorting is ascending and I wanted to get a quotient of sets. The test passed by accident.

1 {'j'}  # I was getting this element
2 {'i', 'j'}

0 set()
1 {'j'}  # after 39d01b7 (fix) I'm getting this (same element, but the logic is fixed)

@mciszczon
Copy link
Collaborator

@m-aciek I'd say this is not a minor change, as it changes the internal behavior of the library: from "match first" to "match best"—which also might not be a trivial thing to say what it means that one structure matches best and not the other.

I could envision this new behavior as an opt-in feature, i.e. union_deep_match=True somewhere to enable deep processing of unions. This would come with a performance hit as well, I believe.

So my proposed solution is:

  • implement this deep union matching behind an opt-in flag
  • ensure dacite behaves same way as before if no flag is set
  • create extensive test suites to cover the new functionality and ensure no incidental breaking changes
  • add performance tests to track the performance of deep union matching

I know it's quite a lot and would love to help, but struggle to find the time. Would you be willing to try to add/finish all of the pieces described above?

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

Successfully merging this pull request may close these issues.

When parsing with Union Optional values are swallowing the input
3 participants