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

Flatten AND and OR conditions #173

Merged
merged 3 commits into from
Jan 19, 2024
Merged

Flatten AND and OR conditions #173

merged 3 commits into from
Jan 19, 2024

Conversation

ojii
Copy link
Contributor

@ojii ojii commented Jan 4, 2024

Previously, AND and OR conditions with more than two elements would lead to nested ANDs and ORs, making the resulting expression much more complicated and harder to read/understand. This change flattens AndCondition and OrCondition objects when they're combined.

Context: had to debug some complicated conditions (~10 AND's) and the extreme nesting makes it super hard to debug/read them.

Not a huge fan of the name MinLen2AppendOnlyList so if someone has a better suggestion I'd love to hear it.

Previously, AND and OR conditions with more than two elements would lead
to nested ANDs and ORs, making the resulting expression much more
complicated and harder to read/understand. This change flattens
AndCondition and OrCondition objects when they're combined.
@ojii ojii force-pushed the flattened-conditions branch from 12e13d8 to ce6e63f Compare January 4, 2024 09:57
src/aiodynamo/utils.py Outdated Show resolved Hide resolved
src/aiodynamo/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

Overall thoughts, sorry if imprecise.

I feel that the only use for the new class is the isinstance test.
Everything else is a regular tuple.
Would it be better to subclass a plain tuple instead?

@dimaqq
Copy link
Contributor

dimaqq commented Jan 5, 2024

Overall thoughts, sorry if imprecise.

I feel that the only use for the new class is the isinstance test. Everything else is a regular tuple. Would it be better to subclass a plain tuple instead?

And I managed to get it all wrong.
Would it be better to change And/OrCondition's lhs/rhs to children which is a tuple (or a list)?

dimaqq added a commit to dimaqq/aiodynamo that referenced this pull request Jan 5, 2024
@dimaqq
Copy link
Contributor

dimaqq commented Jan 5, 2024

I've tried to express my thoughts on refactoring in #174, pull it in if you deem it worthy 🙇🏻

src/aiodynamo/utils.py Outdated Show resolved Hide resolved
@dimaqq dimaqq mentioned this pull request Jan 5, 2024
@ojii
Copy link
Contributor Author

ojii commented Jan 5, 2024

MinLen2AppendOnlyList needlessly generic, renamed to SubConditions and de-genericized.

@ojii ojii merged commit b628c31 into master Jan 19, 2024
54 checks passed
@ojii ojii deleted the flattened-conditions branch January 19, 2024 12:06
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