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

Use the class name in the __repr__ implementations #465

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

dlax
Copy link
Contributor

@dlax dlax commented Nov 4, 2024

Previously, the __repr__() method of those settings source classes was inherited from InitSettingsSource's thus returning a misleading 'InitSettingsSource(init_kwargs={})'. We here define a specific __repr__() method implementation for classes using a config file (json, toml, yaml).

@dlax dlax force-pushed the sources-repr branch 2 times, most recently from 0e18a54 to 6e226a6 Compare November 4, 2024 13:13
@dlax dlax changed the title Define __repr__() on (Json|Toml|Yaml)ConfigSettingsSource Fix repr() for (Json|Toml|Yaml)ConfigSettingsSource Nov 4, 2024
@dlax dlax marked this pull request as ready for review November 4, 2024 13:21
tests/test_settings.py Outdated Show resolved Hide resolved
@hramezani
Copy link
Member

Thanks @dlax for the PR. I left a comment

@dlax dlax requested a review from hramezani November 6, 2024 08:25
Viicos
Viicos previously requested changes Nov 6, 2024
Copy link
Member

@Viicos Viicos left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Instead, let's change the implementation of InitSettingsSource.__repr__ to:

    def __repr__(self) -> str:
        return f'{self.__class__.__name__}(init_kwargs={self.init_kwargs!r})'

@hramezani
Copy link
Member

init_kwargs

Then we will have JsonConfigSettingsSource(init_kwargs={}) for json config source. we don't have init_kwargs in this source.

@Viicos
Copy link
Member

Viicos commented Nov 6, 2024

Ah yes, well in this case probably best to keep it as is, but use self.__class__.__name__ for each __repr__ implementation to keep the class name and repr in sync

@dlax
Copy link
Contributor Author

dlax commented Nov 6, 2024 via email

@hramezani
Copy link
Member

@dlax Please can you change all the __repr__ to and use self.__class__.__name__?

@Viicos Viicos changed the title Fix repr() for (Json|Toml|Yaml)ConfigSettingsSource Use the class name in the __repr__ implementations Nov 6, 2024
@dlax
Copy link
Contributor Author

dlax commented Nov 6, 2024 via email

tests/test_settings.py Outdated Show resolved Hide resolved
tests/test_settings.py Outdated Show resolved Hide resolved
tests/test_settings.py Outdated Show resolved Hide resolved
dlax added 2 commits November 6, 2024 16:40
Previously, the __repr__() method of those settings source classes was
inherited from InitSettingsSource's thus returning a misleading
'InitSettingsSource(init_kwargs={})'. We here define a specific __repr__()
method implementation for classes using a config file (json, toml,
yaml).
@hramezani hramezani dismissed Viicos’s stale review November 6, 2024 20:26

I approved it

@hramezani hramezani merged commit 87ad4db into pydantic:main Nov 6, 2024
21 checks passed
@hramezani
Copy link
Member

Thanks @dlax

@dlax dlax deleted the sources-repr branch November 7, 2024 08:05
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.

3 participants