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.

Sign up for GitHub

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

Jump to bottom

Feature/instance level source customization #350

Closed
wants to merge 6 commits into from
Closed

Feature/instance level source customization #350

wants to merge 6 commits into from

Conversation

acederberg
Copy link

I have made this pull request to close my own issue, #346.

I essentially wanted instance level source customization, therefore I added the callback to _settings_customize_instance.

This change should also help users when testing since they will not have to add settings_customize_sources to their class, and will only have to pass _settings_customize_instance to the constructor.

This also includes a small modification to pyproject.toml to define test dependencies to improve the developer experience.

Sorry that black went wild on settings.py, there are only 3 lines actually added in there. I am assuming that actions will probably fix this, if not, I will fix it.

@acederberg
wip(sources): Support for instance level source customization.
392ecfc 
Added ``tests`` under optional depends because it is annoying to run
``pytest`` multiple times to know which depends must be installed.
@acederberg
fix(tests): Added versions to test dependencies.
4bd10d6
@acederberg acederberg mentioned this pull request Jul 23, 2024
Open
@hramezani
Copy link
Member

Thanks @acederberg for this PR. please:

hramezani
hramezani reviewed Jul 23, 2024
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
@acederberg
Copy link
Author

@hramezani I added tests and docs, it looks like everything is good to go. I notice that the tests fail for 3.8 because of the use of tuple type hints - yet such type hints are included in the source. I will fix this so that tests pass but am concerned about this inconsistency.

@acederberg
Copy link
Author

@hramezani This should work. Is there anything else I ought to do?

@hramezani
Copy link
Member

Thanks @acederberg for the update.

I need to check it more with the team. not sure this is the best way to support it.

@hramezani
Copy link
Member

hramezani commented Jul 29, 2024 •
edited
Loading

@acederberg Unfortunately, we are going not to accept this PR because:

  • Supporting instance-level source customization needs some structural work that I prefer to do it in V3
  • This is a specific use case and adding a new param does not make sense here. I know that pydantic-settings basically doesn't support it, but it doesn't make sense to add a new param now and probably remove it in V3

I think we can keep the issue open to think about it in V3

Thanks for your effort here and sorry for this

@acederberg
Copy link
Author

@hramezani No worries, I understand - these are good reasons.

Is there any way that I could help with v3?

@hramezani
Copy link
Member

@hramezani No worries, I understand - these are good reasons.

Is there any way that I could help with v3?

Thanks for your understanding ❤️

Right now, we don't have any specific plan for V3. but we can keep the issue open and V3 tag to it to consider it when we are going to work on V3.

I am going to close the PR.

Thanks for your effort

@hramezani hramezani closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hramezani hramezani hramezani left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

More __init__ Overwrite Support
2 participants
@acederberg @hramezani