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

Use incrementing ids for expression keys #553

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mitchheddles
Copy link

@mitchheddles mitchheddles commented Jul 7, 2023

Fixes #94

I've introduced a new ExpressionId class which will generate auto incrementing ids for expressions.

@mitchheddles
Copy link
Author

@jeremydaly if you're okay with this approach I can update the unit test assertions

@naorpeled
Copy link
Collaborator

@mitchheddles
thanks for the PR, you're awesome!

I think that this approach is a bit problematic, debugging wise.
I'll need to give this more thought.

@naorpeled
Copy link
Collaborator

naorpeled commented Jul 8, 2023

Few months ago I started this PR,
wdyt about that approach?
if you like it, feel free to continue it,
unfortunately I don't have enough time to finish it.

@mitchheddles
Copy link
Author

mitchheddles commented Jul 8, 2023

@mitchheddles thanks for the PR, you're awesome!

I think that this approach is a bit problematic, debugging wise. I'll need to give this more thought.

Are you referring to my first implementation with the find and replace, or the newest version?

Is your concern that attr0 is impossible to debug when an error is thrown? If so, we could do something similar to what you have but with an incrementing counter to ensue uniqueness.

@naorpeled
Copy link
Collaborator

@mitchheddles thanks for the PR, you're awesome!

I think that this approach is a bit problematic, debugging wise. I'll need to give this more thought.

Are you referring to my first implementation with the find and replace, or the newest version?

Is your concern that attr0 is impossible to debug when an error is thrown? If so, we could do something similar to what you have but with an incrementing counter to ensue uniqueness.

I'm referring to both.
Regarding the counter, we need to keep in mind that the payload to DDB should be as minimal as possible so I'm not entirely sure we should do that.
Let me know what you think

@mitchheddles
Copy link
Author

@naorpeled what if this was a configuration option that’s off by default? People can opt into it.

Alternatively, we might need custom error handling so we can show more detailed messaging.

@naorpeled
Copy link
Collaborator

@naorpeled what if this was a configuration option that’s off by default? People can opt into it.

Alternatively, we might need custom error handling so we can show more detailed messaging.

Hmm, would need to discuss this with @ThomasAribart and @jeremydaly.

@mitchheddles
Copy link
Author

@naorpeled please let me know what you decide, I’m keen to keep this one moving

@naorpeled
Copy link
Collaborator

@naorpeled please let me know what you decide, I’m keen to keep this one moving

Haven't received a reply yet,
will keep you posted ❤️

Appreciate your energy, you rock!

@mitchheddles
Copy link
Author

@naorpeled please let me know what you decide, I’m keen to keep this one moving

Haven't received a reply yet, will keep you posted ❤️

Appreciate your energy, you rock!

Hey mate, has there been any news yet? We’re currently using a patched version of this library in production for a few weeks now and haven’t had any troubles. It would be great to get this released properly.

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.

Update using dashes
2 participants