Skip to content
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

Add index check command #28

Merged
merged 3 commits into from
Apr 5, 2023
Merged

Add index check command #28

merged 3 commits into from
Apr 5, 2023

Conversation

madninja
Copy link
Member

@madninja madninja commented Apr 3, 2023

No description provided.

@madninja madninja force-pushed the madninja/index_check branch from 1185716 to 00b9a5f Compare April 4, 2023 19:34
@JayKickliter
Copy link
Contributor

The upcoming overlaps command should return a non-zero exit code if overlaps are detected. Do you should find should also return non-zero if not found?

Comment on lines +170 to +171
input: path::PathBuf,
cells: Vec<h3ron::H3Cell>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're doing here, and why you have to choose between a PathBuf, Vec<H3Cell> or Vec<PathBuf>, H3Cell due to positional arguments. But my preference would be to have a single Cell and and Vec<PathBuf>. This way, the user can call find <SOMEHEX> path/to/indicies/* or even find <SOMEHEX> path/to/indicies/*.h3idz.

Note that #29 uses Vec<PathBuf>, so we should use the same convention regardless which we choose.

Copy link
Member Author

@madninja madninja Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for a folder argument which would be searched for all h3idz files to look for multiple cells in. This seems to be how we've been wanting to use it to find multiple locations at once.

Open to being convinced otherwise

@madninja madninja merged commit a471707 into main Apr 5, 2023
@madninja madninja deleted the madninja/index_check branch April 5, 2023 12:18
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.

2 participants