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

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

Open inean opened this issue 1 year ago • 5 comments

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

inean avatar Sep 02 '24 16:09 inean

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 avatar Sep 02 '24 17:09 hramezani

@inean is it going to support simple cases like:

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

config = Config()

by TOP__0=foo?

hramezani avatar Sep 03 '24 18:09 hramezani

~~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 avatar Sep 03 '24 20:09 inean

@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

inean avatar Sep 03 '24 20:09 inean

@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

hramezani avatar Sep 07 '24 17:09 hramezani