Make header methods publicly readable by michaelkirk · Pull Request #110 · stadiamaps/pmtiles-rs

michaelkirk · 2026-02-17T23:05:49Z

Followup to #108, in which I attempted to add a CLI bin to this crate. There was a bit of friction, so instead I've extracted the CLI code to https://github.com/michaelkirk/pmtiles-rs-cli

Still, it requires some modest changes to the library code.

This was in pursuit of adding a CLI client for the pmtiles-rs crate. See github.com/michaelkirk/pmtiles-rs-cli

Copilot

Pull request overview

This PR exposes additional read-only Header information via public accessor methods, to support external consumers (e.g., the extracted CLI tool) without making the underlying fields publicly mutable.

Changes:

Add public getters on Header for spec version, addressed/entry/content tile counts, clustering flag, and internal directory compression.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-17T23:08:11Z

src/header.rs

        }
    }
+
+    /// Get the version specification of the archive


Doc comment wording here is a bit unclear/grammatically awkward ("version specification"), and it’s missing a trailing period. Consider rephrasing to something like “Get the PMTiles spec version of the archive.” for clearer public API docs.

Suggested change

/// Get the version specification of the archive

/// Get the PMTiles spec version of the archive.

please apply this - it seems you created this PR either from an org or without giving maintainers edit rights. Otherwise good to go

I did previously, but then pedantic clippy got mad, so I reverted it, because it didn't seem worth it.

I'll dig around for an appropriate allow rule so we can all feel really good about this change.

codecov · 2026-02-17T23:08:41Z

Codecov Report

❌ Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.84%. Comparing base (739fa21) to head (640afeb).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/header.rs	0.00%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
- Coverage   78.84%   77.84%   -1.00%     
==========================================
  Files          11       11              
  Lines        1404     1422      +18     
  Branches     1404     1422      +18     
==========================================
  Hits         1107     1107              
- Misses        192      210      +18     
  Partials      105      105

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lseelenbinder · 2026-02-19T12:07:28Z

Looks good to me @michaelkirk. Why the use of #[must_use], though? I don't think that's standard practice for functions that don't return a Result.

michaelkirk · 2026-02-19T17:59:12Z

🤓 Methods that return Result get must_use "for free" since it's annotated on the Result type. There's no need to annotate methods that return a Result with must_use.

I was trying to follow existing patterns, but I don't really care either way. I see now that the code base isn't entirely consistent about this.

must_use getters:

pmtiles-rs/src/tile.rs

Line 106 in 739fa21

#[must_use]
pmtiles-rs/src/tile.rs

Line 150 in 739fa21

#[must_use]
pmtiles-rs/src/directory.rs

Line 66 in 739fa21

#[must_use]
pmtiles-rs/src/header.rs

Line 104 in 739fa21

#[must_use]
plus more

getters without must_use:

pmtiles-rs/src/header.rs

Line 219 in 739fa21

fn read_coordinate_part<B: Buf>(mut buf: B) -> f64 {
(probably some others...)

FWIW, my own thinking is that must_use is helpful for certain kinds of methods, but it's silly to add must_use to every single method. I've never tried to write out the "rules" for when to use must_use, but off the cuff:

to ensure errors aren't ignored (we get this for free with Result type)
to disambiguate mutating methods from "return a mutated copy methods", e.g. geo crate's scale vs scale_mut. If you don't know about scale_mut you might assume scale is a mutating method. We marked it as must_use to help avoid this case. This rationale doesn't apply to getters.

Do you want me to remove the must_use annotations from this PR?

nyurik · 2026-02-19T18:00:54Z

must_use use is usually driven by clippy complaining :)

nyurik · 2026-02-19T18:01:12Z

i would just keep it if it makes clippy happy

michaelkirk · 2026-02-19T18:33:41Z

Indeed clippy complains if they are removed. It makes me question the projects use of clippy pedantic, but that's out of scope for this PR.

nyurik · 2026-02-19T18:40:11Z

heh, one of those minor pesky things... i did find must_use useful as a user of the lib, and its a small price to pay i guess

nyurik · 2026-02-19T19:16:35Z

src/header.rs

        }
    }
+
+    #[allow(clippy::doc_markdown)]


why not simply fix the doc comment with backticks?

I had McDonald's for lunch.

I had McDonalds for lunch.

I'm writing documentation for people, not to appease clippy!

It just so happens that ~~PMTiles~~ PMTiles is conventionally capitalized in a way that it looks like a type in rust, but I'm referring to the proper noun, not to any code element (we don't even have a PMTiles type).

All that said, I don't really care that much. If you want me to write docs for clippy rather than people, I'll do it!

are you unhappy with our machine overlord?!? Borgs won't be happy with you... You should have had Burger King... that said, if you look in the code base, all PMTiles have backticks around them in the docs - keeps things simpler :)

nyurik · 2026-02-19T19:17:52Z

P.S. @lseelenbinder as mentioned above, I wouldn't worry about a few extra must_use - they did save me a few times, not really worth excluding that lint

lseelenbinder · 2026-02-19T19:37:11Z

Agreed @nyurik! Let's keep clippy happy. 😃

I was mainly curious.

## 🤖 New release * `pmtiles`: 0.19.2 -> 0.20.0 (⚠ API breaking changes) ### ⚠ `pmtiles` breaking changes ```text --- failure enum_variant_added: enum variant added on exhaustive enum --- Description: A publicly-visible enum without #[non_exhaustive] has a new variant. ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/enum_variant_added.ron Failed in: variant PmtError:InvalidTileId in /tmp/.tmpxfPXJO/pmtiles-rs/src/error.rs:93 ``` <details><summary>Changelog</summary> <blockquote> ## [0.20.0](v0.19.2...v0.20.0) - 2026-02-20 ### Other - Make header methods publicly readable ([#110](#110)) - Return Result from TileId::new instead of Option ([#107](#107)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).

Make header methods publicly readable

178760c

This was in pursuit of adding a CLI client for the pmtiles-rs crate. See github.com/michaelkirk/pmtiles-rs-cli

Copilot AI review requested due to automatic review settings February 17, 2026 23:05

Copilot started reviewing on behalf of michaelkirk February 17, 2026 23:06 View session

michaelkirk mentioned this pull request Feb 17, 2026

Add CLI with show command for inspecting PMTiles archives #108

Closed

Copilot AI reviewed Feb 17, 2026

View reviewed changes

michaelkirk force-pushed the mkirk/api-for-show-cli branch from ba750ec to 178760c Compare February 17, 2026 23:23

fixup! Make header methods publicly readable

0f08cbb

nyurik reviewed Feb 19, 2026

View reviewed changes

fixup! Make header methods publicly readable

640afeb

lseelenbinder approved these changes Feb 20, 2026

View reviewed changes

nyurik merged commit babb11e into stadiamaps:main Feb 20, 2026
7 of 8 checks passed

lseelenbinder mentioned this pull request Feb 20, 2026

chore: release v0.20.0 #109

Merged

	/// Get the version specification of the archive
	/// Get the PMTiles spec version of the archive.

Comments

Conversation

michaelkirk commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

nyurik Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

michaelkirk Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

michaelkirk Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lseelenbinder commented Feb 19, 2026

Uh oh!

michaelkirk commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nyurik commented Feb 19, 2026

Uh oh!

nyurik commented Feb 19, 2026

Uh oh!

michaelkirk commented Feb 19, 2026

Uh oh!

nyurik commented Feb 19, 2026

Uh oh!

nyurik Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

michaelkirk Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

nyurik Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

michaelkirk Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

nyurik commented Feb 19, 2026

Uh oh!

lseelenbinder commented Feb 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 17, 2026 •

edited

Loading

michaelkirk commented Feb 19, 2026 •

edited

Loading