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

added time encoder #912

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

added time encoder #912

wants to merge 1 commit into from

Conversation

07pepa
Copy link
Member

@07pepa 07pepa commented Apr 7, 2024

I have added time encoded however i suspect there may be issue in pydantic ( speed date) itself see pydantic/speedate#62

@07pepa 07pepa mentioned this pull request Apr 7, 2024
Closed
@roman-right
Copy link
Member

Hi @07pepa ,
THank you for the PR. It looks interesting. I'll check it out this weekend

@07pepa 07pepa force-pushed the add-time-endoder branch 2 times, most recently from 36be7ab to 47a6c77 Compare April 17, 2024 19:36
pepe-rtmlab
pepe-rtmlab reviewed Apr 19, 2024
View reviewed changes
beanie/odm/utils/encoder.py Outdated Show resolved Hide resolved
tests/odm/test_encoder.py Show resolved Hide resolved
@roman-right
Copy link
Member

Hi @07pepa ,
Could you please check the red tests?

@07pepa 07pepa force-pushed the add-time-endoder branch 8 times, most recently from 8087d7c to b0e59ae Compare May 2, 2024 20:27
@07pepa
Copy link
Member Author

07pepa commented May 2, 2024

now is green https://github.com/07pepa/beanie/actions/runs/8930039487/job/24529213097

@07pepa 07pepa mentioned this pull request May 2, 2024
Closed
@07pepa
Copy link
Member Author

07pepa commented Jun 5, 2024

lets wait how pydantic will resolve this here pydantic/pydantic#9571

@roman-right
Copy link
Member

lets wait how pydantic will resolve this here
Yes, let's wait. THank you!

Riverfount
Riverfount previously approved these changes Jul 2, 2024
View reviewed changes
Copy link

@Riverfount Riverfount left a comment

Choose a reason for hiding this comment

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

I read the code and I think everything is fine. My question is why GitHub Actions doesn't run the jobs. But, if the tests are passed, I approve this PR.

@07pepa
Copy link
Member Author

07pepa commented Jul 2, 2024

@Riverfount there is preaty nasty bug in pydantic/pydantic#9571 we should wait how they resolve it IMHO it would be bad if implementation would differ from theirs and it currently does and bypasses pydantic

@07pepa
Copy link
Member Author

07pepa commented Jul 2, 2024

@Riverfount can you remove you approve ? i know this is not usual request... but... in this case is better to wait

@07pepa 07pepa requested a review from Riverfount July 2, 2024 14:42
@Riverfount Riverfount removed their request for review July 2, 2024 14:51
Riverfount
Riverfount reviewed Jul 2, 2024
View reviewed changes
Copy link

@Riverfount Riverfount left a comment

Choose a reason for hiding this comment

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

Waiting pydantic bug fix.

@Riverfount Riverfount dismissed their stale review July 2, 2024 14:55

Waiting pydantic bug fix.

@07pepa 07pepa marked this pull request as draft July 30, 2024 10:08
@github-actions GitHub Actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale label Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Riverfount Riverfount Riverfount left review comments

@pepe-rtmlab pepe-rtmlab pepe-rtmlab left review comments

At least 3 approving reviews are required to merge this pull request.

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

Successfully merging this pull request may close these issues.
4 participants
@07pepa @roman-right @Riverfount @pepe-rtmlab