8000 First part of unified `h3` command by dfellis · Pull Request #497 · uber/h3 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

First part of unified h3 command #497

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

Merged
merged 4 commits into from
Jul 16, 2021
Merged

First part of unified h3 command #497

merged 4 commits into from
Jul 16, 2021

Conversation

dfellis
Copy link
Collaborator
@dfellis dfellis commented Jul 8, 2021

Only two functions implemented so far, but this demonstrates the potential pattern possible. This makes the sub-command names case-insensitive and forces sub-commands to be in explicit locations in the argv list. (Not just hardwired to argv[1] in case we ever need sub-commands of sub-commands).

Work to tackle:

  • All of the rest of the public H3 functions as sub-commands
  • KML output (and input?) where appropriate
  • GeoJSON output (and input?) where appropriate

These can be in follow-up PRs, I hope?

@coveralls
Copy link
coveralls commented Jul 8, 2021

Coverage Status

Coverage remained the same at 98.991% when pulling 8bc69a8 on dfellis:master into 5203711 on uber:master.

@ajfriend
Copy link
Contributor

This looks great! One question though. Do we have a way to add tests for these commands?

@dfellis
Copy link
Collaborator Author
dfellis commented Jul 12, 2021

This looks great! One question though. Do we have a way to add tests for these commands?

It would be integration testing (from the perspective of CMake) that we need to write. I have used shellspec to do this relatively cleanly in other projects. Do we want to potentially use that here but have it actively needed only if you run something like make bdd so most users don't have to download that?

Thoughts @isaacbrodsky @ajfriend @nrabinowitz ? (In my own usage, I have the Makefile acquire the version of shellspec I want to use as a dependency for running the BDD tests so it could also be done without requiring changes to setup for github actions or in local development.)

@isaacbrodsky
Copy link
Collaborator

I don't have any concerns with using an additional tool for testing the CLI. Using shellspec seems reasonable.

@dfellis dfellis merged commit f084317 into uber:master Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0