Skip to content

add timedelta type #137

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

Merged
merged 15 commits into from
Jul 5, 2022
Merged

add timedelta type #137

merged 15 commits into from
Jul 5, 2022

Conversation

fcfangcc
Copy link
Contributor

#122 implement timedelta

@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #137 (8ae3817) into main (3e30540) will increase coverage by 0.05%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
+ Coverage   97.66%   97.71%   +0.05%     
==========================================
  Files          43       44       +1     
  Lines        4231     4422     +191     
  Branches       30       31       +1     
==========================================
+ Hits         4132     4321     +189     
- Misses         99      101       +2     
Impacted Files Coverage Δ
src/input/mod.rs 100.00% <ø> (ø)
src/errors/kinds.rs 98.90% <50.00%> (-1.10%) ⬇️
src/input/datetime.rs 97.79% <97.26%> (-0.20%) ⬇️
pydantic_core/_types.py 100.00% <100.00%> (ø)
src/input/input_abstract.rs 100.00% <100.00%> (ø)
src/input/input_json.rs 96.44% <100.00%> (+0.34%) ⬆️
src/input/input_python.rs 98.39% <100.00%> (+0.08%) ⬆️
src/validators/mod.rs 98.05% <100.00%> (+0.01%) ⬆️
src/validators/timedelta.rs 100.00% <100.00%> (ø)
src/validators/function.rs 96.87% <0.00%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e30540...8ae3817. Read the comment docs.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

This is looking great, thank so much.

A few things here, you also need to update _types.py.

@fcfangcc
Copy link
Contributor Author

@samuelcolvin These problems were fixed

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

looking good, a few more things to fix.

I might merge #138 which will conflict with this, if so I'll fix conflicts for you.

@samuelcolvin
Copy link
Member

also worth noting that while testing this, I found some int overflow bugs speedate, see pydantic/speedate#11 I'll make a new release of speedate in the coming days.

@fcfangcc
Copy link
Contributor Author

fcfangcc commented Jul 2, 2022

@samuelcolvin when fix it , found a speedate bug ,look pydantic/speedate#12

@samuelcolvin
Copy link
Member

I've released speedate v0.5.0 which fixes both those problems.

@samuelcolvin
Copy link
Member

I've merged #138 which will conflict with this quite a lot in terms of code usage. I tried to push to your PR to resolve those conflicts, but I got permission denied - looks like you might have disabled me from contributing to your PR.

To get those changes, you might want to cherry-pick cf39b83.

@fcfangcc
Copy link
Contributor Author

fcfangcc commented Jul 2, 2022

@samuelcolvin I don't know, i have do nothing...... i will try resolve those conflicts

@fcfangcc
Copy link
Contributor Author

fcfangcc commented Jul 2, 2022

I found the settings. Now you can push my branch.

@samuelcolvin
Copy link
Member

great, I've pushed, should save your resolving those changes.

@fcfangcc
Copy link
Contributor Author

fcfangcc commented Jul 4, 2022

This is my first rust PR,unskilled in rust.
pytimedelta_as_timedelta logic is less human, but I have no better idea.
if you have batter way, please optimization it, happy to learn .

@samuelcolvin
Copy link
Member

This is my first rust PR,unskilled in rust.

Then I'm really impressed by what you've done here!

I've updated pytimedelta_as_timedelta, part of it was that I found PyDeltaAccess by chance, also I've written more tests for it - this stuff is not simple it took me a lot of attempts to get it right.

I've also resolved conflicts.

I'll have another look later, but I think this is nearly ready to merge 🎉 .

@samuelcolvin samuelcolvin merged commit 158ab9a into pydantic:main Jul 5, 2022
@samuelcolvin
Copy link
Member

samuelcolvin commented Jul 5, 2022

Thanks so much for all your work on this. 🙏

Feel free to work on other issues which interest you, or create an issue if you think you see a bug or potential feature.

@samuelcolvin samuelcolvin mentioned this pull request Jul 5, 2022
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.

2 participants