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

Fix: Transaction not doing double serialization of the condition #25

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

Conversation

krewx
Copy link

@krewx krewx commented Sep 20, 2024

While doing several updates using transactions it was noticed that the value of the condition was getting serialized two times. With these fix you can ask for the double serialization or just disable one.

@nayaverdier
Copy link
Owner

@krewx Thanks for the PR! Do you see any reason to keep the double serialization around or can we remove it altogether rather than adding a flag?

@krewx
Copy link
Author

krewx commented Sep 20, 2024

I guess that adding the flag would give support for anyone using the function with the double serialization although it should be failing... So I would vote for removing the code... Let me adjust that

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (49b4a33) to head (80a7ce1).
Report is 3 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #25   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          669       685   +16     
  Branches       127       133    +6     
=========================================
+ Hits           669       685   +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nayaverdier
Copy link
Owner

No need for you to do this if you don't want to, I can try to do it sometime in the next week or two. It'd be good to add some test cases that would have failed before, but are succeeding with this fix. Looks like all the condition checks in the transaction tests are just key existence hence why this wasn't caught before.

@krewx
Copy link
Author

krewx commented Sep 20, 2024

I have updated the PR with just removing the double serialization... And let me think about the test cases but that's going to be at another PR jajaja

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