settings-doc icon indicating copy to clipboard operation
settings-doc copied to clipboard

Handle AliasChoices in validation_alias for settings

Open tazlin opened this issue 4 months ago • 2 comments

  • Support for AliasChoices objects in validation_alias, yielding the first choice as the preferred alias.
    • The first, and presumably preferred alias, is guaranteed by pydantic to be the first element of the choices list. Assuming it is in fact a string, this presumably would be a valid default value for most use cases.
    • I considered, and dismissed, the idea of seeking the first instance of a str, but I suspect that that may lead to surprising behavior for existing users of the library. For example, if the first and common/primary alias is an AliasPath, then emitting a secondary alias might end up being unexpected or undesired. As resolving AliasPaths is probably outside of the scope of settings-doc due to the inherent complexity, and existing users likely accept this limitation, I think it is preferable to only emit a value when the first element is a str.
  • Adds new test cases and fixtures for AliasChoices and AliasPath combinations to ensure intended behavior and continued error logging in the case that an AliasPath is the first member of the AliasChoice list.
    • Changes the existing validation_alias for ValidationAliasChoicesSettings.logging_level to be consistent with the pattern in the testing objects for other validation_alias, especially the parametrized fixtures.
  • Adjusted the relevant error message to print the causing object and field, which may help in conveying to users of the library what is causing the message and highlights which objects are not supported for aliases.

This PR is made with full ignorance to what the exact reasons were to not support AliasChoices in the first place. I couldn't come up with a compelling reason why, but I recognize there may be good cause for not supporting it. It is fairly obvious that there was a conscious reason at the time, considering lack of support is explicitly tested for. In the case I've overlooked something in either the code submitted or my underlying reasoning for the PR, please feel free to let me know and I'll be happy to address.

tazlin avatar Aug 15 '25 20:08 tazlin

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 100.00%. Comparing base (1d885b0) to head (7ea9427).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #50   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          187       189    +2     
  Branches        52        53    +1     
=========================================
+ Hits           187       189    +2     
Flag Coverage Δ
integration_tests 80.42% <25.00%> (-0.87%) :arrow_down:
total 100.00% <100.00%> (ø)
unit_tests 74.60% <100.00%> (+0.27%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Aug 15 '25 20:08 codecov[bot]

If there is anything I can do to improve the PR for review, or if its just dead on arrival, feel free to let me know. I understand that you likely have other priorities. I'm only bumping for visibility - feel no pressure beyond a gentle nudge. I will happily maintain a fork for my purposes until this is merged.

tazlin avatar Aug 27 '25 20:08 tazlin