-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New API for date-like types unit/format configuration #11504
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
base: main
Are you sure you want to change the base?
Conversation
@ollz272, would love your feedback on this API proposal! Want to solidify things before I implement logic in |
CodSpeed Performance ReportMerging #11504 will not alter performanceComparing Summary
|
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.
These settings seem simple and powerful to me 👍
Hi! Take with a pinch of salt as I only have access to my phone at the moment! However looking at this, all seems sensible and will achieve what we’re looking for! really excited to see this in 2.11! |
I'm about to go on leave for a couple of weeks so if any other input from me is wanted i won't be around, @benjsec may be able to give some input my absence. Really looking forward to seeing this! |
After much discussion with the team, we've settled on the above, which closely matches some of the work that @ollz272 did previously, so I'll port that over in |
Unfortunate update here - I'll be scaling back my work on Sorry for the churn on this feature request - it definitely fell to the bottom of our todo list a few times as features like improving the alias configuration API took priority due to popular demand. I'll leave this PR open for now (maybe someone will have the capacity to pick this up in the next month or so). @Viicos or @davidhewitt, feel free to close for cleanup purposes as you see fit (it's linked to the relevant issues, so should be easy enough to dig up later). |
Hi @sydney-runkle, slowly coming back now. Unfortunate that this didn’t make it into 2.11, but understandable. Will this be prioritised internally for 2.12? Or will you be relying on community PR’s to push this along? |
@davidhewitt @Viicos Hi! I saw on linkedin that sydney has moved on from pydantic so maybe she wasn't the best person to ask the above question to, sorry if you guys aren't either! Same question: Will this be prioritised internally for 2.12? Or will you be relying on community PR’s to push this along? |
Hi @ollz272 sorry for the slow response. We'll tackle this in 2.12. It might be that we recommend using validators instead (unless not possible or incurs performance issues, I'll need to take a deeper look), as we want to avoid introducing many configuration values that affect validation behavior as this can get quickly out of hand. |
Thanks! I think the main thing i was concerned with in the original issue was serialisation, so im not sure a validator would fix that. Open to other suggestions on how this could work, ofc, but really we want to avoid things like this: T_SerializedDatetime = Annotated[
dt.datetime,
PlainSerializer(
lambda x: x.timestamp() * 1000,
return_type=int,
when_used="json",
),
]
class Value(BaseModel):
series: dict[T_SerializedDatetime, float] as the cost of computing the |
Hi @DouweM, i think from looking around you’re now managing pydantic! Congrats! I was just hoping to get some insight into progress on this issue/pr and if we can expect it in 2.12? |
Hi @ollz272 , yes this will definitely be tackled for 2.12. I'm not fully working on Pydantic these days but I'll get back around it before the release for sure. |
Fixes #10454
Fixes #7940
Previously outlined this API in #10949, but continuing here with a clean history.