Handle AliasChoices in validation_alias for settings#50
Open
tazlin wants to merge 1 commit intoradeklat:mainfrom
Open
Handle AliasChoices in validation_alias for settings#50tazlin wants to merge 1 commit intoradeklat:mainfrom
tazlin wants to merge 1 commit intoradeklat:mainfrom
Conversation
25e898f to
10fa013
Compare
- Support for `AliasChoices` objects in `validation_alias`, yielding the first choice as the preferred alias. - 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.
10fa013 to
7ea9427
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Author
|
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. |
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.
AliasChoicesobjects invalidation_alias, yielding the first choice as the preferred alias.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 anAliasPath, 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 astr.AliasChoicesandAliasPathcombinations to ensure intended behavior and continued error logging in the case that anAliasPathis the first member of theAliasChoicelist.validation_aliasforValidationAliasChoicesSettings.logging_levelto be consistent with the pattern in the testing objects for othervalidation_alias, especially the parametrized fixtures.This PR is made with full ignorance to what the exact reasons were to not support
AliasChoicesin 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.