search

apache / incubator-graphar

An open source, standard data file format for graph data storage and retrieval.
https://graphar.apache.org/
Apache License 2.0
192 stars 40 forks source link

fix(c++): using C++17's nested namespaces #489

Closed ywh555hhh closed 1 month ago

ywh555hhh commented 1 month ago

Initial modifications to feat(c++): Use C++17's nested namespaces #473 completed, involving content of anonymous namespace and content in result.hpp not changed yet

Reason for this PR

to slove the issue #473

What changes are included in this PR?

Most cpp folders need to use cpp17 nested loops

Are these changes tested?

No local tests

Are there any user-facing changes?

no

ywh555hhh commented 1 month ago

I seem to be ignoring some of the call logic for nested namespaces and I'll fix it myself

acezen commented 1 month ago

Hi @ywh555hhh, thanks for the work, the CI indicate that the code format check is failed. Please follow the https://github.com/apache/incubator-graphar/blob/87d5eeb6a7f86574328da26f08d950e3c5bc3de8/.github/workflows/ci.yml#L82-L83 to install clang-format 8.0 and fix the format with

make graphar-clformat
ywh555hhh commented 1 month ago

I encountered an error when trying to open a project in a container using vscode's remote container plug-in. How can I solve it?

Error response from daemon: pull access denied for registry.cn-hongkong.aliyuncs.com/graphscope/graphar-dev, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

acezen commented 1 month ago

I encountered an error when trying to open a project in a container using vscode's remote container plug-in. How can I solve it?

Error response from daemon: pull access denied for registry.cn-hongkong.aliyuncs.com/graphscope/graphar-dev, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

It seems that the image has been expired, I have re-pushed it, can you try again?

ywh555hhh commented 1 month ago

info2.txt I've tried it with both macos and linux and it still doesn't seem to work

acezen commented 1 month ago

info2.txt I've tried it with both macos and linux and it still doesn't seem to work

It seems that the image is a private repository, thanks for report the error. I have created an issue #491 to fix this.

acezen commented 1 month ago

info2.txt I've tried it with both macos and linux and it still doesn't seem to work

It seems that the image is a private repository, thanks for report the error. I have created an issue #491 to fix this.

Hi, @ywh555hhh, the image has been updated, could you rebase the main and try again? sorry for the obstruction.

ywh555hhh commented 1 month ago

I'm happy to contribute to the community, but I also want to know how to solve the formatting problem. If I can't use docker right now, it means I need to fix the problem on macOS

acezen commented 1 month ago

'm happy to contribute to the community, but I also want to know how to solve the formatting problem. If I can't use docker right now, it means I need to fix the problem on macOS

the clang-format-8 is also available for macos:

curl -L https://github.com/muttleyxd/clang-tools-static-binaries/releases/download/master-22538c65/clang-format-8_macosx-amd64 --output clang-format

or you can edit the code directly and fix the format base on the information provided by CI: https://github.com/apache/incubator-graphar/actions/runs/9188486680/job/25270239169?pr=489

acezen commented 1 month ago

It's better not to delete the empty line in the end of the source file. FYI https://stackoverflow.com/questions/2287967/why-is-it-recommended-to-have-empty-line-in-the-end-of-a-source-file

ywh555hhh commented 1 month ago

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

I had no intention of doing that. Could this have been some kind of operational error that I wasn't aware of. Sorry

acezen commented 1 month ago

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

I had no intention of doing that. Could this have been some kind of operational error that I wasn't aware of. Sorry

It would make the CI failed. see https://github.com/apache/incubator-graphar/actions/runs/9190636917/job/25275644225?pr=489

You can revert it to the original commit with git https://stackoverflow.com/questions/215718/how-can-i-reset-or-revert-a-file-to-a-specific-revision

ywh555hhh commented 1 month ago

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

I had no intention of doing that. Could this have been some kind of operational error that I wasn't aware of. Sorry

It would make the CI failed. see https://github.com/apache/incubator-graphar/actions/runs/9190636917/job/25275644225?pr=489

You can revert it to the original commit with git https://stackoverflow.com/questions/215718/how-can-i-reset-or-revert-a-file-to-a-specific-revision

Thanks for the Pointers. I really appreciate you taking the time to mentor me

acezen commented 1 month ago

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

I had no intention of doing that. Could this have been some kind of operational error that I wasn't aware of. Sorry

It would make the CI failed. see https://github.com/apache/incubator-graphar/actions/runs/9190636917/job/25275644225?pr=489 You can revert it to the original commit with git https://stackoverflow.com/questions/215718/how-can-i-reset-or-revert-a-file-to-a-specific-revision

Thanks for the Pointers. I really appreciate you taking the time to mentor me

You are welcome. We are pleased to have your contribution.

ywh555hhh commented 1 month ago

I'll give clang-format-8 a try

ywh555hhh commented 1 month ago

The version of clang-format I used was wrong and I will try to install the required version myself

ywh555hhh commented 1 month ago 
yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp %

I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard

acezen commented 1 month ago 
yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp %

I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard

the command 

make graphar-clformat

is run in the build directory if you create a build dir

ywh555hhh commented 1 month ago 
yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp %

I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard

the command

make graphar-clformat

is run in the build directory if you create a build dir

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % cd ..
yiweihan@yiweihandeMacBook-Air incubator-graphar % mkdir build
yiweihan@yiweihandeMacBook-Air incubator-graphar % cd build
yiweihan@yiweihandeMacBook-Air build % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air build %

Did I misunderstand something? I don't even seem to be looking for the Makefile

acezen commented 1 month ago 
yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp %

I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard

the command

make graphar-clformat

is run in the build directory if you create a build dir

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % cd ..
yiweihan@yiweihandeMacBook-Air incubator-graphar % mkdir build
yiweihan@yiweihandeMacBook-Air incubator-graphar % cd build
yiweihan@yiweihandeMacBook-Air build % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air build %

Did I misunderstand something? I don't even seem to be looking for the Makefile

you need to run cmake .. before make. It's the same with building process

ywh555hhh commented 1 month ago 
yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp %

I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard

the command

make graphar-clformat

is run in the build directory if you create a build dir

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % cd ..
yiweihan@yiweihandeMacBook-Air incubator-graphar % mkdir build
yiweihan@yiweihandeMacBook-Air incubator-graphar % cd build
yiweihan@yiweihandeMacBook-Air build % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air build %

Did I misunderstand something? I don't even seem to be looking for the Makefile

you need to run cmake .. before make. It's the same with building process

tks I've tried it. Can this be merged now?

ywh555hhh commented 1 month ago

Overall LGTM, and there have some nits to be fixed

ok👌🏻