search

uber / h3

Hexagonal hierarchical geospatial indexing system
https://h3geo.org
Apache License 2.0
4.8k stars 457 forks source link

Implement all of the inspection subcommands for the h3 cli program #837

Closed dfellis closed 3 months ago

dfellis commented 4 months ago

I refactored the macros a bit more to reduce (but not completely eliminate, unfortunately) the redundancy in the code.

So, similar to what @nrabinowitz mentioned in the last PR, the definition of the Arg values for the subcommands is baked into the definition of the subcommands themselves. I was unable to find any way to generate an array of function pointers to these subcommands at the same time the functions were being defined, because you can't inline define them in C and I couldn't figure out any macro trickery to fake it.

Anyways, this should hopefully make binding the rest of the H3 functions much faster (which I think I demonstrated by binding all of the inspection functions excepting maxFaceCount since it's not really meant to be used directly).

dfellis commented 4 months ago

This error doesn't make any sense to me: https://github.com/uber/h3/actions/runs/8947967184/job/24580540663?pr=837#step:4:27

It's saying that H3Index is an unsigned long long but when I try to make the suggested change locally, my compiler is telling me that H3Index is an unsigned long like I originally wrote.

coveralls commented 4 months ago

Coverage Status

coverage: 98.824%. remained the same when pulling 2e752609b7d49d41911f1a300e4f434fedff7d66 on h3-cli-inspection-commands into 3a71b09b5b2d98eca8b1e8349117e611ab8cb8fa on master.

grim7reaper commented 4 months ago

It's saying that H3Index is an unsigned long long but when I try to make the suggested change locally, my compiler is telling me that H3Index is an unsigned long like I originally wrote.

Could be because the system/toolchain used in GitHub CI map uint64_t tounsigned long long and your system map it onto unsigned long. IIRC, that's why the C99 standard library provides PRIu64 format specifier: to be portable. Code should probably be:

printf("%"PRIu64, c);

That will resolve to lu or llu according to what the system expects.

nrabinowitz commented 4 months ago

Sure, sgtm

On Sun, May 5, 2024 at 7:23 PM David Ellis @.***> wrote:

@.**** commented on this pull request.

In CMakeTests.cmake https://github.com/uber/h3/pull/837#discussion_r1590483233:

@@ -231,6 +231,15 @@ add_h3_test(testCellToBBoxExhaustive src/apps/testapps/testCellToBBoxExhaustive. add_h3_cli_test(testCliCellToLatLng "cellToLatLng -c 8928342e20fffff" "POINT(-122.5003039349 37.5012466151)") add_h3_cli_test(testCliLatLngToCell "latLngToCell --lat 20 --lng 123 -r 2" "824b9ffffffffff") add_h3_cli_test(testCliCellToBoundary "cellToBoundary -c 8928342e20fffff" "POLYGON((-122.4990471431 37.4997389893, -122.4979805011 37.5014245698, -122.4992373065 37.5029321860, -122.5015607527 37.5027541980, -122.5026273256 37.5010686174, -122.5013705214 37.4995610248, -122.4990471431 37.4997389893))") +add_h3_cli_test(testCliGetResolution "getResolution -c 85283473fffffff" "5") +add_h3_cli_test(testCliGetBaseCellNumber "getBaseCellNumber -c 85283473fffffff" "20") +add_h3_cli_test(testCliStringToInt "stringToInt -c 85283473fffffff" "599686042433355775") +add_h3_cli_test(testCliIntToString "intToString -c 599686042433355775" "85283473fffffff") +add_h3_cli_test(testCliIsValidCell "isValidCell -c 85283473fffffff" "true") +add_h3_cli_test(testCliIsNotValidCell "isValidCell -c 85283473ffff" "false") +add_h3_cli_test(testCliIsResClassIII "isResClassIII -c 85283473fffffff" "true") +add_h3_cli_test(testCliIsPentagon "isPentagon -c 85283473fffffff" "false") +add_h3_cli_test(testCliGetIcosahedronFaces "getIcosahedronFaces -c 81743ffffffffff" "3, 8, 13, 9, 4")

I don't remember who introduced CLI testing in this way, actually. I am pretty sure that I didn't, but I might be wrong about it.

In any case, I was following the existing convention and there was no objection before on this, but I agree it feels weird.

Do you mind if I take a stab at restructuring these tests during the next PR?

— Reply to this email directly, view it on GitHub https://github.com/uber/h3/pull/837#discussion_r1590483233, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABIAZDB3AC7AZJIUNTPYALZA3SSJAVCNFSM6AAAAABHGQANU6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZZHA4DGNZTGY . You are receiving this because you were mentioned.Message ID: @.***>