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

✨ adding new type iban #65

Closed
wants to merge 16 commits into from
Closed

✨ adding new type iban #65

wants to merge 16 commits into from

Conversation

kroncatti
Copy link

@kroncatti kroncatti commented Jun 18, 2023 •
edited
Loading

Resolves #10

  • Adding IBAN type along with its properties

Selected Reviewer: @yezz123

@kroncatti kroncatti mentioned this pull request Jun 18, 2023
Open
@kroncatti kroncatti marked this pull request as draft June 21, 2023 00:35
@yezz123 yezz123 marked this pull request as ready for review June 24, 2023 23:51
@yezz123 yezz123 self-assigned this Jun 24, 2023
@yezz123 yezz123 added the enhancement New feature or request label Jun 24, 2023
@yezz123
Copy link
Collaborator

yezz123 commented Jun 25, 2023

please review

@yezz123 yezz123 requested a review from Kludex June 25, 2023 00:06
@kroncatti
Copy link
Author

We have more properties that could be added as shown in here. Do you think it's worth it ?

@yezz123
Copy link
Collaborator

yezz123 commented Jun 29, 2023

We have more properties that could be added as shown in here. Do you think it's worth it ?

Yes its totally worth it 👍🏻

@kroncatti
Copy link
Author

I'll do it still on this PR then, thanks. Possibly today.

@codecov
Copy link

codecov bot commented Jul 2, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5216db3) 100.00% compared to head (ed611a0) 100.00%.

Additional details and impacted files 
@@            Coverage Diff            @@
##              main       #65   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        11    +1     
  Lines          665       726   +61     
  Branches       166       183   +17     
=========================================
+ Hits           665       726   +61

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kroncatti
Copy link
Author

Just did @yezz123, sorry for the time it took. If I did not miss anything, we should be good.

@kroncatti kroncatti requested a review from yezz123 July 2, 2023 14:13
kroncatti
kroncatti commented Jul 2, 2023
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
@kroncatti
Copy link
Author

@yezz123 now we're ready for review 😄

@kroncatti
Copy link
Author

Resolves #10

@samuelcolvin
Copy link
Member

Resolves #10

please put this in the PR body so it closes the issue upon merge.

@kroncatti
Copy link
Author

Done @samuelcolvin, thanks.

@yezz123
Copy link
Collaborator

yezz123 commented Jul 5, 2023

sorry for late review @kroncatti i will review it again today and test it then we can merge it 🙏🏻

yezz123
yezz123 approved these changes Jul 6, 2023
View reviewed changes
Copy link
Collaborator

@yezz123 yezz123 left a comment

Choose a reason for hiding this comment

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

@kroncatti Thank you for the great work 🙏🏻

I test it and tried multiple examples.

Will merge it once @tpdorsey approve for the documentation

docs/iban_types.md Outdated Show resolved Hide resolved
@yezz123 yezz123 requested review from tpdorsey and removed request for Kludex July 6, 2023 08:52
@yezz123 yezz123 mentioned this pull request Jul 6, 2023
Closed
5 tasks
lig
lig suggested changes Jul 6, 2023
View reviewed changes
pydantic_extra_types/iban.py Outdated Show resolved Hide resolved
@Kludex
Copy link
Member

Kludex commented Jul 6, 2023

Please don't merge this yet.

Let's try to get __get_pydantic_core_schema__ merged into https://github.com/mdomke/schwifty/ first.

@lig lig requested a review from Kludex July 6, 2023 12:05
@lig
Copy link
Contributor

lig commented Jul 6, 2023

@Kludex I've just request a review from you on this. You have control now over when it could be merged.

@Kludex Kludex mentioned this pull request Jul 6, 2023
Closed
@Kludex
Copy link
Member

Kludex commented Jul 6, 2023

@Kludex I've just request a review from you on this. You have control now over when it could be merged.

So much power! 👀

I've opened mdomke/schwifty#147 on schwifty. 🙏

@kroncatti
Copy link
Author

Amazing folks! thanks!

@Kludex
Copy link
Member

Kludex commented Jul 6, 2023

Amazing folks! thanks!

dnd :)

@kroncatti
Copy link
Author

Hey folks,

It's been a few weeks we are waiting on this. What is you guys opinion ? Should we wait to merge ?

@Kludex
Copy link
Member

Kludex commented Aug 7, 2023

Hey folks,

It's been a few weeks we are waiting on this. What is you guys opinion ? Should we wait to merge ?

I think we should wait for mdomke/schwifty#147.

@waveman68
Copy link

Hey folks,

It's been a few weeks we are waiting on this. What is you guys opinion ? Should we wait to merge ?

I vote to move forward.

@yezz123
Copy link
Collaborator

yezz123 commented Jan 9, 2024

This PR is ready once we drop support for Python 3.7 🙏🏻 .

cc @kroncatti @Kludex

@yezz123 yezz123 added the Types label Jan 9, 2024
@yezz123 yezz123 mentioned this pull request Jan 9, 2024
Closed
@mdomke
Copy link

mdomke commented Jan 9, 2024 •
edited
Loading

@kroncatti @yezz123 @Kludex Hi! I've implemented the Pydantic protocol support in mdomke/schwifty#147 last year November. Sorry that it took me a while, but I was quite busy at the time. Is the implementation in schwifty working for you or is there something that you need additionally, so that you have to create this wrapper type?

@Kludex
Copy link
Member

Kludex commented Jan 9, 2024 •
edited
Loading

Oh, nice! Maybe we can add that to the Pydantic documentation?

@Kludex Kludex closed this Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lig lig lig requested changes

@yezz123 yezz123 yezz123 approved these changes

@tpdorsey tpdorsey Awaiting requested review from tpdorsey

@Kludex Kludex Awaiting requested review from Kludex

Assignees

@yezz123 yezz123

Labels
enhancement New feature or request ready for review Types
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add IBAN Type to Types
7 participants
@kroncatti @yezz123 @samuelcolvin @Kludex @lig @waveman68 @mdomke