-
Notifications
You must be signed in to change notification settings - Fork 44
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
[C++][Improvement] Redesign and unify the implementation of validation in C++ Writer/Builder #186
Conversation
🎊 PR Preview 6ac637a has been successfully built and deployed to https://alibaba-graphar-build-pr-186.surge.sh 🤖 By surge-preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the job, it's overall look good to me. there are some places I think we can revise:
- In your design, the
default_validate
is invalid to writer/builder's validate level, so we need to note this in comment to users and add a check if validate_level isdefault_validate
or not in constructors. - It seems that weak/strong's check is different for different operations. Beside the brief doc for
ValidateLevel
, how about add some docs to the operations that what willweak
andstrong
check for the operation? I think that would help develop and user to understand the operation's behavior. - There are some codes not follow the google c++ code style, please fix them.
Thanks for your suggestions. For the second comment, weak/strong check is consistent for different operations now, strong_validate will also check the schema, while weak_validate only validates index, count, adj_list type, property group and size. |
The code seems to meet the clang-format config, no need to refine. |
I have checked the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Proposed changes
We need to develop a more comprehensive and consistent approach to validation across all of GraphAr's tools and libraries. As the first step, this change redesigned and unified the implementation of validation for
Writer
andBuilder
in the C++ library.Validate
interface to users and usevalidate_level
as a parameter of the writing operationsWriter
andBuilder
Clear()
method to high-levelBuilder
, enabling to add data and then dump for multiple roundsValidateLevel
typeTypes of changes
What types of changes does your code introduce to GraphAr?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
Solve #183