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

Feature: new mbtiles diff command #1068

Merged
merged 9 commits into from
Feb 8, 2024
Merged

Feature: new mbtiles diff command #1068

merged 9 commits into from
Feb 8, 2024

Conversation

sharkAndshark
Copy link
Collaborator

@sharkAndshark sharkAndshark commented Dec 15, 2023

Implement mbtiles diff file_a.mbtiles file_b.mbtiles diff.mbtiles. This should behave exactly the same as mbtiles copy file_a.mbtiles --diff-with-file file_b.mbtiles diff.mbtiles.

  • Add mbtiles diff as a separate command
  • Update doc

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

good luck, a fun little project :) I left a few minor comments, but i'm sure lots will change by the end

mbtiles/src/bin/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/diff.rs Outdated Show resolved Hide resolved
mbtiles/src/diff.rs Outdated Show resolved Hide resolved
mbtiles/src/diff.rs Outdated Show resolved Hide resolved
@nyurik nyurik changed the title Make diff a separated command Feature: new mbtiles diff command Dec 16, 2023
@nyurik
Copy link
Member

nyurik commented Dec 16, 2023

I just did a bit of refactoring to separate Clap args from the internals of the mbtiles copy options. You could simply reuse that directly, without any changes to the diff. This way the new CLI command will be available right away, and then we can do more refactorings later.

@nyurik
Copy link
Member

nyurik commented Feb 2, 2024

@sharkAndshark i merged in the main branch, resolving a few conflicts. Let me know if you need any help with this

@sharkAndshark
Copy link
Collaborator Author

@nyurik Thx!! :D Traveling with family, will be back Monday

mbtiles/src/bin/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/bin/mbtiles.rs Outdated Show resolved Hide resolved
mbtiles/src/bin/mbtiles.rs Outdated Show resolved Hide resolved
@nyurik
Copy link
Member

nyurik commented Feb 7, 2024

Looks much better, thx! TODO: docs and tests

@sharkAndshark sharkAndshark marked this pull request as ready for review February 8, 2024 02:16
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

looking better and better. For testing, please also add another portion to https://github.com/maplibre/martin/blob/main/tests/test.sh#L456

Something basic like

  $MBTILES_BIN diff \
    ./tests/fixtures/mbtiles/world_cities.mbtiles \
    ./tests/fixtures/mbtiles/world_cities_modified.mbtiles \
    "$TEST_TEMP_DIR/world_cities_diff2.mbtiles" \
    2>&1 | tee "$TEST_OUT_DIR/copy_diff2.txt"

docs/src/mbtiles-diff.md Outdated Show resolved Hide resolved
docs/src/mbtiles-diff.md Outdated Show resolved Hide resolved
mbtiles/src/bin/mbtiles.rs Outdated Show resolved Hide resolved
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

Thanks! I think its ready to go.

@nyurik nyurik enabled auto-merge (squash) February 8, 2024 02:58
@nyurik nyurik merged commit 79a8912 into maplibre:main Feb 8, 2024
19 checks passed
@sharkAndshark sharkAndshark deleted the diff branch March 14, 2024 08:16
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