[Feat][C++] Add isValid for property in high level

apache / incubator-graphar

An open source, standard data file format for graph data storage and retrieval.

https://graphar.apache.org/

Apache License 2.0

195 stars 40 forks source link

[Feat][C++] Add isValid for property in high level #435

Closed jasinliu closed 2 months ago

jasinliu commented 3 months ago

Proposed changes

Related to #434

Checklist

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.

[x] I have read the CONTRIBUTING doc
[x] I have signed the CLA
[ ] Lint and unit tests pass locally with my changes
[ ] I have added tests that prove my fix is effective or that my feature works
[x] I have added necessary documentation (if appropriate)

Further comments

If it works, i will add test case for it.

jasinliu commented 3 months ago

The method and the result turn to this format.

acezen commented 3 months ago

Hi, @jasinliu, Can you add this IsValid to Edge too? I think they both need the API for property value check

jasinliu commented 3 months ago

Hi, @jasinliu, Can you add this IsValid to Edge too? I think they both need the API for property value check

Sure! I think IsValid is added to Edge property. I will add some error handlers, docs and test cases.

acezen commented 3 months ago

Hi, @jasinliu, Can you add this IsValid to Edge too? I think they both need the API for property value check

Sure! I think IsValid is added to Edge property. I will add some error handlers, docs and test cases.

That would be great. When you think the PR is ready, feel free to ping me or @lixueclaire to merged it.

jasinliu commented 3 months ago

I need a review due to that the test runs fine on my ubuntu machine, but fails on the action @acezen @lixueclaire

lixueclaire commented 3 months ago

I need a review due to that the test runs fine on my ubuntu machine, but fails on the action @acezen @lixueclaire

hi, @jasinliu Without detailed error logs, it's challenging to diagnose the exact problem. Could you try incrementally commenting out sections of the test to isolate and identify the failing component?

jasinliu commented 3 months ago

hi, @jasinliu Without detailed error logs, it's challenging to diagnose the exact problem. Could you try incrementally commenting out sections of the test to isolate and identify the failing component?

OK. Let me try

jasinliu commented 3 months ago

hi, @jasinliu Without detailed error logs, it's challenging to diagnose the exact problem. Could you try incrementally commenting out sections of the test to isolate and identify the failing component?

OK. Let me try

The problem is the data in gar-test with absolute path. I want to remove it.Here

acezen commented 2 months ago

hi, @jasinliu Without detailed error logs, it's challenging to diagnose the exact problem. Could you try incrementally commenting out sections of the test to isolate and identify the failing component?

OK. Let me try

The problem is the data in gar-test with absolute path. I want to remove it.Here

The testing update PR has been merged, you can update the testing submodule and run the CI again.

acezen commented 2 months ago

@jasinliu you may need to checkout the testing to the latest commit to apply your change in testing repo.