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

Revert "Merge branch 'format_types'" #463

Closed
wants to merge 3 commits into from

Conversation

andrewbaxter
Copy link

This reverts commit ed5dab2, reversing changes made to 93c8601.

There are some manual changes to include reverts of subsequent mrs, plus some manual fixups. It builds, but I don't know if it's correct yet.

Related to discussion in #446

andrew added 2 commits January 4, 2024 22:19
This reverts commit ed5dab2, reversing
changes made to 93c8601.
@andrewbaxter
Copy link
Author

Okay AFAICT this works (no compile errors for storage or kms at least). I'm not sure if I should regenerate everything in this MR or as a separate MR afterwards or what.

@Byron
Copy link
Owner

Byron commented Jan 4, 2024

Thanks a lot for going through with it!

I think this tests expectation could be fixed, the failure appears genuine. Could you do that?

Thanks again for giving it another shot.

I'm not sure if I should regenerate everything in this MR or as a separate MR afterwards or what.

Please do not regenerate anything, such contributions are impossible to review. I regenerate myself before making a release.

@andrewbaxter
Copy link
Author

I think this tests expectation could be fixed, the failure appears genuine. Could you do that?

Ah yep. I think I pushed a fix, but I'll take another look tomorrow if there's still issues.

Please do not regenerate anything, such contributions are impossible to review. I regenerate myself before making a release.

Okay perfect, thanks!

@andrewbaxter
Copy link
Author

Sorry, I'll have to look at that failure tomorrow.

@Byron
Copy link
Owner

Byron commented Jan 4, 2024

No worries, this failure is expected, some python breakage that just happens without any change.

I think this PR will be merged soon.

@andrewbaxter
Copy link
Author

Oh, per the comment #446 it sounds like going with standard base64 might be the way to go? Or are you still thinking that's risky?

@Byron
Copy link
Owner

Byron commented Jan 4, 2024

After having caught up, and given your experience by now with the codebase, I think you could try to keep what is there locally, change the encoding to base64 and that should solve all problems, right?

If not, we can still merge this PR and probably leave the encoding/decoding to the user.

I will wait until I hear from you.

@Byron Byron marked this pull request as draft January 4, 2024 17:42
@andrewbaxter
Copy link
Author

Closed in favor of #446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants