remove binary sv2 serde #1459
remove binary sv2 serde #1459
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1459 +/- ##
==========================================
- Coverage 19.34% 18.06% -1.29%
==========================================
Files 144 127 -17
Lines 10963 9555 -1408
==========================================
- Hits 2121 1726 -395
+ Misses 8842 7829 -1013
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Shourya742
force-pushed
the
2025-02-06-remove-binary_sv2_serde
branch
2 times, most recently
from
94bf09f to
934abff
Compare
Shourya742
force-pushed
the
2025-02-06-remove-binary_sv2_serde
branch
from
934abff to
e175bd3
Compare
plebhash
force-pushed
the
2025-02-06-remove-binary_sv2_serde
branch
from
e175bd3 to
93bef4a
Compare
|
rebasing due to #1455 (comment) |
there's no point in having two directories named binary_sv2 anymore
plebhash
force-pushed
the
2025-02-06-remove-binary_sv2_serde
branch
from
9419e27 to
9511a53
Compare
There was a problem hiding this comment.
there's still things to be done here
@Shourya742 please finish 67e8e51, there's some details on the commit message
also, the code is still full of references to serde (comments, README)
as discussed in a call with @Shourya742 , we are going to proceed the following way:
|
Shourya742
force-pushed
the
2025-02-06-remove-binary_sv2_serde
branch
2 times, most recently
from
da5edc2 to
390561e
Compare
no point in keeping this anymore remove core from docs.yaml
Shourya742
force-pushed
the
2025-02-06-remove-binary_sv2_serde
branch
from
390561e to
b0aee13
Compare
protocols/v2/binary-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Show resolved
Hide resolved
| @@ -163,8 +156,7 @@ impl<'a, T: GetSize> GetSize for Seq0255<'a, T> { | |||
| } | |||
|
|
|||
| /// [`Seq064K`] represents a sequence with a maximum length of 65535 elements. | |||
| /// This structure uses a generic type `T` and a lifetime parameter `'a` | |||
| /// to ensure compatibility with `serde-sv2`. | |||
| /// This structure uses a generic type `T` and a lifetime parameter `'a`. | |||
There was a problem hiding this comment.
given that the original motivation for the lifetime parameter was serde-sv2 compatibility, shouldn't we also do something about that?
it seems now there's no real motivation for this anymore, and simply removing the comment will make it hard for us to understand in the future
There was a problem hiding this comment.
Yes, we will remove the lifetime parameters in the refactor binary_sv2 PR. I just didn't touch them in this PR since we agreed it would bloat the scope. Thanks for pointing it out for other reviewers to keep in mind while reviewing this and the binary_sv2 PR!
| @@ -455,12 +447,12 @@ impl<'a, const ISFIXED: bool, const SIZE: usize, const HEADERSIZE: usize, const | |||
| } | |||
| } | |||
|
|
|||
| /// The liftime is here only for type compatibility with serde-sv2 | |||
| /// The lifetime 'a is defined. | |||
There was a problem hiding this comment.
given that the original motivation for the lifetime parameter was serde-sv2 compatibility, shouldn't we also do something about that?
it seems now there's no real motivation for this anymore, and simply removing the comment will make it hard for us to understand in the future
There was a problem hiding this comment.
Yes, we will remove the lifetime parameters in the refactor binary_sv2 PR. I just didn't touch them in this PR since we agreed it would bloat the scope. Thanks for pointing it out for other reviewers to keep in mind while reviewing this and the binary_sv2 PR!
Shourya742
force-pushed
the
2025-02-06-remove-binary_sv2_serde
branch
from
b0aee13 to
684ca23
Compare
…ich dont require it
- Add reg-test-block.toml to make the unit test work again - Add serde as dev dependency to make the toml importing work.
Shourya742
force-pushed
the
2025-02-06-remove-binary_sv2_serde
branch
from
684ca23 to
49c78e3
Compare
part of: #1387