Skip to content

Comments

Add ConfigResolver with necessary tests#641

Merged
ubaskota merged 3 commits intosmithy-lang:config_resolution_mainfrom
ubaskota:config_resolver
Feb 23, 2026
Merged

Add ConfigResolver with necessary tests#641
ubaskota merged 3 commits intosmithy-lang:config_resolution_mainfrom
ubaskota:config_resolver

Conversation

@ubaskota
Copy link

@ubaskota ubaskota commented Feb 20, 2026

Issue #, if available:

Description of changes:

Implements ConfigResolver to resolve configuration values from multiple sources in precedence order. The resolver iterates through sources and returns the first non-None value found, along with the source name for provenance tracking. Returns (None, None) when no source provides a value.

Tests
Added unit tests covering:

  • Single and multiple source resolution
  • Source precedence ordering
  • Unresolved value handling
  • Tuple return format validation

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ubaskota ubaskota requested a review from a team as a code owner February 20, 2026 00:08
Copy link
Contributor

@jonathan343 jonathan343 left a comment

Choose a reason for hiding this comment

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

Thanks Ujjwal.

I left some minor comments and suggestions. However, my biggest concern with this PR is the approach for config resolver sharing.

  1. It doesn't follow any of the approaches discussed in previous designs
  2. In cases where multiple threads hit the function before the first result is cached, it will result in each building their own resolver which is unnecessary/duplicate work

"""Initialize the resolver with sources in precedence order.
:param sources: List of configuration sources in precedence order. The first
source in the list has the highest priority. The list is copied to
Copy link
Contributor

Choose a reason for hiding this comment

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

Copying it seems a little odd to me; we could simplify this a bit by removing the copy. This kind of goes against the concept of "responsible users" in Python.

The short version is that the Python community is full of responsible users who generally understand that if a property or method is prefixed with an underscore they should not set/use it directly. Those who ignore that are doing so at their own risk.

While I don't expect this to ever matter in practice, the copy here is a net negative for me; it creates additional overhead (both memory-wise and cognitive overhead) which complicated the code without adding value.

That said, it's a single line and not worth a lot of back and forth.

@ubaskota ubaskota changed the title Add ConfigResolver with shared singleton pattern Add ConfigResolver with necessary tests Feb 23, 2026
assert region == ("us-west-2", "source_one")
assert retry_mode == ("adaptive", "source_two")

def test_returns_non_string_values(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this test is a bit odd to me; I'm not sure what's different about string values that makes them special. I'd think this would be more like "test_resolved_returns_different_types", then we test a string, an int, a bool, and whatever else.

These test make it look like string is the default type that a resolver will return, and I don't think that's true.

@ubaskota ubaskota merged commit 318519f into smithy-lang:config_resolution_main Feb 23, 2026
4 checks passed
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.

4 participants