-
Notifications
You must be signed in to change notification settings - Fork 247
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
make field_name
and data
available on ValidationInfo
#980
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #980 +/- ##
==========================================
- Coverage 93.08% 92.99% -0.10%
==========================================
Files 106 106
Lines 15832 15844 +12
Branches 26 35 +9
==========================================
- Hits 14738 14734 -4
- Misses 1087 1103 +16
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #980 will not alter performanceComparing Summary
|
############################################################################### | ||
# All this stuff is deprecated by #980 and will be removed eventually | ||
# They're kept because some code external code will be using them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just remove these and make it a breaking change. Bump the major version if you think we need to (I don't think we do, I'd like to keep considering pydanic-core
as a quasi implementation detail)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep these since there will be some reasonable external use of them, and a deprecating warning is a lot better than an ImportError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I'll put the attributes behind __getattr__
so there's a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since there will be some reasonable external use of them
is there any data to back this up? we've made plenty of breaking changes in pydantic-core
and to my knowledge no one has complained, so either no one is impacted or those that are impacted understand the state of the library and what guarantees we are making and not.
What I want to avoid here is complexity and technical debt. Using a __getattr__
is the opposite of the direction I am suggesting we go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes there is, the original issues that reported this used FieldValidationInfo
, also:
and more, see this github search
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm okay, you've convinced me there's some usage. I guess we can leave this complexity in here, such a bummer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think this is a more fundamental change to core-schema than we've made before, hence worrying about compatibility, I think most other changes have either been additions, or are behind the public functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also used functions from the core_schema.*
namespace when adapting Pydantic V2 support for immoney, and now quite surprised to see this usage is already deprecated (I haven't even had time to fully adopt V2 across all libs yet, and API is already changing). What would have been the preferred option to use here, is there any way I could have avoided using these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll, we had to make this change to make pydantic V2 more compatible with v1 while still fully typed.
AFAIK this is the only post V2 change where we introduced incompatible changes.
It's unfortunate, but I don't see how we could have avoided it, or you could have predicted it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks for providing insight. Unfortunate and some extra churn for me, but I assume it's for the better in the long run 👍
3591b68
to
0544b12
Compare
See pydantic/pydantic#7542 for more details.