Document Feature and RelationMember fields in feature.proto - #632
Merged
Conversation
FeatureToIndex was well-documented after the centroid_s2_cell_id change,
but Feature and RelationMember had real gaps for a newcomer:
- `tags` gave no clue it's flattened (key_id, value_id) StringPool-index
pairs; that fact was only written down in a private helper's rustdoc
in assemble.rs, invisible from the proto itself.
- `way_members` and `RelationMember.id` look parallel (both uint64,
one message apart) but use different ID encodings: way_members holds
raw OSM node IDs, RelationMember.id holds `id`-encoded feature IDs.
Nothing warned a reader of that.
- `RelationMember.role` had no comment at all -- not obviously a
StringPool index rather than an enum or literal role string.
- version/changeset/timestamp were undocumented: no units, no note
that they default to 0 when OSM doesn't supply them.
- geometry_wkb didn't say it's little-endian WKB with (lon, lat) order.
- No message-level note on which fields apply to which OSM element kind.
Also gave Feature the same "this is internal/ephemeral, never sent over
the wire or stored long-term, field numbers are free to be renumbered"
note FeatureToIndex already had -- and, since Feature is declared first,
moved the full explanation there and had FeatureToIndex just refer back
to it, so a top-to-bottom reader sees the full story once, in order.
Fixed two pre-existing typos in FeatureToIndex's trailing comment
("should't" -> "shouldn't", "finalm" -> "final") while in the area.
Comment-only change; no behavior change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Comment-only change: documents every field of
FeatureandRelationMemberinfeature.proto. Prompted by reviewing doc clarity after #631 landedcentroid_s2_cell_id/coverage_s2_cell_idonFeatureToIndex— that message ended up well-documented, butFeature/RelationMemberhad real gaps.Gaps fixed
tags: gave no clue it's flattened(key_id, value_id)StringPool-index pairs. That fact only existed in a private helper's rustdoc inassemble.rs, invisible from the proto itself.way_membersvsRelationMember.id: bothuint64, one message apart, look parallel — butway_membersholds raw OSM node IDs whileRelationMember.idholdsid-encoded feature IDs (osm_id*10 + {1,2,3}). Nothing warned a reader they're differently encoded.RelationMember.role: no comment at all — not obviously aStringPoolindex rather than an enum or the literal role string.version/changeset/timestamp: undocumented, no units, no note that they default to0when OSM doesn't supply them.geometry_wkb: didn't say it's little-endian WKB with (longitude, latitude) coordinate order.way_membersways-only,relation_membersrelations-only).Also gave
Featurethe same "internal/ephemeral, never sent over the wire or stored long-term, field numbers are free to be renumbered" noteFeatureToIndexalready had. SinceFeatureis declared first in the file, the full explanation lives there andFeatureToIndexjust refers back to it, so a top-to-bottom reader gets the full story once, in the right order.Fixed two pre-existing typos in
FeatureToIndex's trailing comment (should't→shouldn't,finalm→final) while in the area.All field semantics were checked against the actual population code (
assemble_tags,assemble_relation_members,assemble_featureinassemble.rs), not guessed.Testing
cargo build(regenerates the prost bindings from the proto) andcargo fmt --check: clean. No.rschanges, so no behavior change.🤖 Generated with Claude Code