Skip to content

Conversation

CommanderStorm
Copy link
Member

During #1926 I noticed that the way we currently handle this is not super working out.

  • get_unrecognized(&self) -> &UnrecognizedValues is not owned => if I need to merge, this is not quite possible
  • UnrecognizedValues is not quite the information that I need. If I just need the keys, we should just ship around the keys
  • some of the functions related to this are not documented
  • is_default related to this is always true.

@CommanderStorm CommanderStorm self-assigned this Jul 28, 2025
fn get_unrecognized(&self) -> &UnrecognizedValues {
&self.unrecognized
fn get_unrecognized_keys(&self) -> UnrecognizedKeys {
self.unrecognized.keys().cloned().collect()
Copy link
Member

@nyurik nyurik Jul 30, 2025

Choose a reason for hiding this comment

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

why do we need to clone it in every fn? Why not just return the ref to the original, and if needed, the caller can do the .keys().cloned().collect() on the result of the get_unrecognized()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I need this to be owned as otherwise this trait does not work on every level (think enums, or a struct with two sub-structs).
Especially at the top level this otherwise does not work

I considered an iterator (I can chain them, not owned) but this neither works nicely with the prefixes, nor enums.

I doubt that cloning for the warning case is a huge issue.
This is one malloc/free per warning more.

@Copilot Copilot AI review requested due to automatic review settings August 4, 2025 20:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the handling of unrecognized configuration keys by changing the API from returning references to values to returning owned collections of just the keys. The change addresses limitations with the previous approach where merging unrecognized values was difficult and unnecessary data was being passed around.

Key changes:

  • Replace get_unrecognized() -> &UnrecognizedValues with get_unrecognized_keys() -> UnrecognizedKeys
  • Remove the is_default() method from the ConfigExtras trait since it was always returning true
  • Update documentation for SourceConfigExtras methods to clarify their purpose

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
martin/src/config.rs Adds UnrecognizedKeys type alias and updates copy_unrecognized_config to work with keys only
martin/src/file_config.rs Removes is_default() method, updates trait methods, and adds documentation for SourceConfigExtras
martin/src/pg/config.rs Updates finalize() method to return UnrecognizedKeys instead of UnrecognizedValues
martin/src/pmtiles/config.rs Removes is_default() implementation and updates get_unrecognized to get_unrecognized_keys
martin/src/mbtiles/config.rs Updates trait implementation and adds missing parse_urls() method
martin/src/cog/config.rs Updates trait implementation to return keys instead of values reference
martin/src/styles/config.rs Updates trait implementation to return keys instead of values reference
martin/src/sprites/config.rs Updates trait implementation to return keys instead of values reference

@CommanderStorm CommanderStorm requested a review from nyurik August 31, 2025 15:11
@CommanderStorm CommanderStorm force-pushed the document-SourceConfigExtras branch from 41dcd8c to dabe4c8 Compare August 31, 2025 20:50
@CommanderStorm CommanderStorm changed the title chore: refactor how get_unrecognized_keys is handled chore: refactor how get_unrecognized_keys is handled to enable unused config detection Aug 31, 2025
@CommanderStorm CommanderStorm merged commit b4a06c7 into maplibre:main Sep 12, 2025
27 checks passed
@CommanderStorm CommanderStorm deleted the document-SourceConfigExtras branch September 12, 2025 22:04
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