gaborkaszab commented on code in PR #3:
URL: https://github.com/apache/iceberg-cpp/pull/3#discussion_r1865359742
##########
src/demo.cc:
##########
@@ -0,0 +1,26 @@
+/*
Review Comment:
I think I'd structure the code a bit differently. How I imagined the
structure of the c++ lib is something like this:
```
iceberg-cpp/
- api/
- src/
- test/
...
- core/
- src/
- test/
...
- example/
- src/
- test/
```
and so on. (I hope the indentation is displayed as I intended to :) )
So in general I think there should be separate libs for each of the modules
of the implementation and each of them should have their own src/ test/
folders. As I see now these folders are provided on the top level of the
directory structure.
If we want to have an include/ folder for the headers separately, I think we
should also have them on the module-level and not at the top level.
What do you think?
##########
src/demo.cc:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
What I don't get now is the include/ folder. Does that meant to hold the
header files that clients of this lib could include? Isn't it something that is
called 'API' in the Java implementation? If yes, could we rename it to 'api'?
If no, then I think there is no point of separating thos headers from the cc
files, they could live next to each other.
Could you please help me understand this?
##########
include/CMakeLists.txt:
##########
@@ -0,0 +1,22 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
I'm wondering about the purpose of the 'include' folder. IS this for storing
all the .h files separately from the .cc files? I find it easier to navigate if
the headers are next to the cc files tbh.
--
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]
