Copilot commented on code in PR #892:
URL: https://github.com/apache/incubator-graphar/pull/892#discussion_r2911472201
##########
cpp/src/graphar/high-level/vertices_builder.h:
##########
@@ -51,21 +52,30 @@ namespace graphar::builder {
*/
class Vertex {
public:
- Vertex() : empty_(true) {}
+ Vertex() = default;
/**
* @brief Initialize the vertex with a given id.
*
* @param id The id of the vertex.
*/
- explicit Vertex(IdType id) : id_(id), empty_(false) {}
+ explicit Vertex(IdType id) : id_(id) {}
/**
* @brief Get id of the vertex.
*
+ * The id is absent until explicitly set or assigned by VerticesBuilder.
+ *
* @return The id of the vertex.
*/
- IdType GetId() const noexcept { return id_; }
+ IdType GetId() const { return id_.value(); }
+
Review Comment:
`GetId()` currently calls `id_.value()`, which throws
`std::bad_optional_access` with an unhelpful default message when the id is
unset (this also propagates as a generic runtime error in the Python bindings).
Consider checking `HasId()` and throwing a more descriptive exception (or
providing a non-throwing accessor) so users get a clear failure reason.
##########
cpp/src/graphar/high-level/vertices_builder.h:
##########
@@ -51,21 +52,30 @@ namespace graphar::builder {
*/
class Vertex {
public:
- Vertex() : empty_(true) {}
+ Vertex() = default;
/**
* @brief Initialize the vertex with a given id.
*
* @param id The id of the vertex.
*/
- explicit Vertex(IdType id) : id_(id), empty_(false) {}
+ explicit Vertex(IdType id) : id_(id) {}
/**
* @brief Get id of the vertex.
*
+ * The id is absent until explicitly set or assigned by VerticesBuilder.
+ *
* @return The id of the vertex.
Review Comment:
The new doc comment says the id may be “assigned by VerticesBuilder”, but it
doesn’t clarify whether that id is relative to the builder (0..N-1) or includes
`start_vertex_index_`. Since `AddVertex(..., index=-1)` assigns
`vertices_.size()` today, consider clarifying this wording to avoid misleading
API users about what id they will observe.
##########
python/test/test_high_level_api.py:
##########
@@ -111,6 +111,16 @@ def test_vertices_builder(sample_graph_vertex):
# Set validate level
builder.SetValidateLevel(ValidateLevel.strong_validate)
+ empty_vertex = BuilderVertex()
+ assert empty_vertex.Empty()
+ with pytest.raises(Exception):
+ empty_vertex.GetId()
+ empty_vertex.SetId(42)
Review Comment:
The test uses `with pytest.raises(Exception)` for `GetId()` on an unset id,
which is very broad and can hide unrelated failures. It would be more robust to
assert the specific exception type raised by the binding (and ideally match the
message) so the test only passes for the intended behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
