feat(treeview): add defaultTreeview helper #156
Conversation
Summary of ChangesHello @GrapeBaBa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the SSZ (Simple Serialize) library by adding a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a defaultTreeView helper function across various SSZ types, which is a great simplification for creating TreeView instances from their default values, especially in tests. The test files have been updated to use this new helper, which cleans up the code nicely.
My review focuses on ensuring the new helper functions adhere to the repository's style guide. I've noticed that the new functions are missing assertions, which are a key part of the style guide for ensuring code safety and correctness. I've added review comments to each new defaultTreeView function with suggestions to add assertions for preconditions. I've also recommended adding postcondition assertions and doc comments to further improve code quality and maintainability.
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(allocator, pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(allocator, pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(allocator, pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(allocator, pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(allocator, pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(allocator, pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(allocator, pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(allocator, pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(allocator, pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(allocator, pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(allocator, pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
This new function should include assertions to comply with the style guide. I've added a precondition assertion for pool.
The style guide recommends an average of two assertions per function. You could also add a postcondition assertion, for example:
const view = try TreeView.init(allocator, pool, root);
std.debug.assert(view.base_view.data.root == root);
return view;Additionally, adding a doc comment would improve code clarity:
/// Creates a TreeView from the default value of this type. pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView {
std.debug.assert(pool != null);
const root = try tree.fromValue(allocator, pool, &default_value);
return try TreeView.init(allocator, pool, root);
}
References
- All function arguments and return values, pre/postconditions and invariants must be asserted. The assertion density of the code must average a minimum of two assertions per function. (link)
There was a problem hiding this comment.
Pull request overview
This PR introduces a defaultTreeView helper method across various SSZ types to simplify test code. The helper creates a TreeView from a default value, reducing boilerplate code that was previously required for initializing, creating tree nodes, and manually constructing TreeView instances.
- Adds
defaultTreeViewhelper methods to all SSZ type implementations (vectors, lists, containers, bit types, and byte types) - Refactors test code across multiple test files to use the new helper method
- Reduces test code complexity by eliminating manual value initialization, tree creation, and cleanup steps
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ssz/type/vector.zig | Adds defaultTreeView helper to FixedVectorType and VariableVectorType |
| src/ssz/type/list.zig | Adds defaultTreeView helper to FixedListType and VariableListType |
| src/ssz/type/container.zig | Adds defaultTreeView helper to FixedContainerType and VariableContainerType |
| src/ssz/type/byte_vector.zig | Adds defaultTreeView helper to ByteVectorType |
| src/ssz/type/byte_list.zig | Adds defaultTreeView helper to ByteListType |
| src/ssz/type/bit_vector.zig | Adds defaultTreeView helper to BitVectorType |
| src/ssz/type/bit_list.zig | Adds defaultTreeView helper to BitListType |
| test/int/ssz/tree_view/list_composite.zig | Refactors tests to use new defaultTreeView helper, replacing manual initialization patterns |
| test/int/ssz/tree_view/list_basic.zig | Simplifies test setup by using defaultTreeView helper |
| test/int/ssz/tree_view/container.zig | Updates test code to use defaultTreeView for cleaner test initialization |
| test/int/ssz/tree_view/bit_vector.zig | Simplifies BitVector test initialization with defaultTreeView |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(pool, &default_value); |
There was a problem hiding this comment.
The tree.fromValue function for FixedContainerType does not accept an allocator parameter. The signature is pub fn fromValue(pool: *Node.Pool, value: *const Type). This implementation incorrectly passes allocator as the first parameter and will cause a compilation error.
The correct implementation should be:
const root = try tree.fromValue(pool, &default_value);| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(pool, &default_value); |
There was a problem hiding this comment.
The tree.fromValue function for FixedVectorType does not accept an allocator parameter. The signature is pub fn fromValue(pool: *Node.Pool, value: *const Type). This implementation incorrectly passes allocator as the first parameter and will cause a compilation error.
The correct implementation should be:
const root = try tree.fromValue(pool, &default_value);
GrapeBaBa
force-pushed
the
gr/default_tree_view
branch
from
82f227e to
47ad6e7
Compare
Signed-off-by: grapebaba <[email protected]>
GrapeBaBa
force-pushed
the
gr/default_tree_view
branch
from
82f227e to
8337442
Compare
spiral-ladder
left a comment
spiral-ladder
left a comment
There was a problem hiding this comment.
LGTM with 1 minor comment
I think gemini's suggestion of asserting pool != null is unnecessary
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | ||
| const root = try tree.fromValue(allocator, pool, &default_value); | ||
| return try TreeView.init(allocator, pool, root); | ||
| } |
There was a problem hiding this comment.
if tree is meant to be the namespace for TreeView related functions, could we put this under tree? Then we use it like foo.tree.defaultTreeView
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | |
| const root = try tree.fromValue(allocator, pool, &default_value); | |
| return try TreeView.init(allocator, pool, root); | |
| } | |
| pub const tree = struct { | |
| ... | |
| ... | |
| pub fn defaultTreeView(allocator: std.mem.Allocator, pool: *Node.Pool) !TreeView { | |
| const root = try tree.fromValue(allocator, pool, &default_value); | |
| return try TreeView.init(allocator, pool, root); | |
| } | |
| }; | |
|
Seems we don't need this, will see if it is needed once #168 merged |
Motivation
Align lodestar
defaultViewDUAPI