Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/support complex list definition on env vars [Closes #376] #377

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

inean
Copy link

@inean inean commented Sep 2, 2024

Proposal of implementation. It works but no sanity check on indexes are done 😅

try:
# check if key is an integer. If so, it's an index. we fake a field info with the list type
# so future calls can continue to traverse the model and set proper types for leaf nodes
int(key)
Copy link
Member

@hramezani hramezani Sep 2, 2024

Choose a reason for hiding this comment

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

we need a test case with non int index

result, values = [], result
for i in (str(i) for i in range(len(values))):
if i not in values:
raise ValueError(f'Expected entry with index {i} for {field_name}')
Copy link
Member

Choose a reason for hiding this comment

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

a test case for this line as well

@hramezani
Copy link
Member

Thanks @inean for this PR. please:

  • Fix lint error
  • Add requested test cases
  • Add a new example in the docs to show the new feature

@hramezani
Copy link
Member

@inean is it going to support simple cases like:

class Config(BaseSettings):
   top: List[str]

config = Config()

by TOP__0=foo?

@inean
Copy link
Author

inean commented Sep 3, 2024

No, only complex cases ie: List[List|Dict|Submodel]. I'm refactoring code to move logic to deep_update so it can handle List[List... those cases. TOP__0=foo iIMHO is redundant and already could be written has TOP=['foo'], but may works as a side effect. let me test tomorrow...

@inean
Copy link
Author

inean commented Sep 3, 2024

@hramezani Just one question:

given this code...

class TopModel(BaseModel):
    v1: str | None = None
    v2: int 
 
class Settings(BaseSettings):
    top:  List[List[TopModel]]

...

env.set(TOP='[[{'v2': 'xx'}]]')
env.set(TOP__0__0__v2 = '1')
env.set(TOP__1__0__v2 = '2')
env.set(TOP__3__0__v2='3')

cfg = Settings()

Should TOP__3__0__v2 be ignored or raise a ValueError? We can append new entries at the end of the list (TOP__1 in this case), but TOP__3 requires a default value as TOP__2which we can't provide. Python in this case (when using [].index on a list with index > len() ) simply appends at the end, but I think that it's better to just ignore that insert or raise error

@hramezani
Copy link
Member

@inean Thanks for the update and sorry for being late.

After discussing with the team about supporting this syntax, we decided not to support it because:

  • The code looks complex and pydantic-settings is already complex. It is not worth adding this complexity to support a feature partially.
  • Also, the feature doesn't work for the simple case that I mentioned above and I am sure people want it in the future if we introduce the new syntax

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