-
Notifications
You must be signed in to change notification settings - Fork 58
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
conversion: replace MarshalSSZTo
with MarshalSSZAppend
and MarshalSSZInto
#171
Conversation
…s is a breaking change
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #171 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 1632 1638 +6
=========================================
+ Hits 1632 1638 +6 |
I'd recommend going through the paces with deprecation instead, or at the very least keeping around a function that preserves the old behavior (under a new symbol). |
Deprecating Possibly, I could remove it, and add it back a few releases later. At least then the breakage would be obvious. And that's perhaps better than what this PR does, which breaks things in a way which will only show in runtime, not compile-time. |
I can't imagine anyone is calling it any way except direct. sszgen/fastssz do not really support external interfaces. The |
Hm, why can't you use type Uint256 uint256.Int` But I agree, perhaps best to just deprecate |
I have now changed this PR to remove @jshufro SGTY? |
MarshalSSZTo
with MarshalSSZAppend
and MarshalSSZInto
This fixes the append vs overwrite part of #170 .
However, this is technically a breaking change. On the other hand, the way it now works is incompatible with the fastssz encoding library. And since nobody has raised this earlier, I assume nobody has really used it in production.
So, should be fine to merge, on that reasoning.
(?)