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
added KafkaDsn
to networks.py
and add default ports for HttpUrl
#2447
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2447 +/- ##
===========================================
- Coverage 100.00% 99.96% -0.04%
===========================================
Files 25 25
Lines 5109 5134 +25
Branches 1050 1051 +1
===========================================
+ Hits 5109 5132 +23
- Misses 0 1 +1
- Partials 0 1 +1
Continue to review full report at Codecov.
|
I think this will also allow a fix for my issue #2450, where I want default ports on http urls. Will a stringification include the defaults? or will the original string be preserved? |
An example of its behaviour is shown in the test for |
@davidolrik I could commit this solution to this PR and link your issue to this ticket. What do you think? |
I've also added default 443 or 80 ports to HttpUrl model which are accessible but not displayed in the stringified version to this PR. |
Just what I was thinking! 😁 Maybe also add test for http urls with ports included which is also one of the defaults? m = Model(v='https://www.example.com:443')
assert m.v.port == '443'
assert m.v == 'https://www.example.com:443' |
I was just playing around with your branch, and I found a bug: m = Model(v='https://www.example.com:1234')
assert m.v.port == '1234'
assert m.v == 'https://www.example.com:1234' Here the port is also set to 443:
|
I didn't notice any test that was not expecting to get a ValidationError in I had already been working on fix by the time I read your message though |
@davidolrik added tests as you requested |
@samuelcolvin ready for review |
@PrettyWood I'd appreciate review and/or some feedback 🛩️ 🙏 |
I couldn't make this work with default port in |
I added some |
I noticed that CI test "test FastAPI" failed. I run through an error message and I not sure if it's caused by my changes.. |
@PrettyWood 🙃 🎉 |
pydantic/networks.py
Outdated
@@ -114,7 +115,7 @@ class AnyUrl(str): | |||
|
|||
@no_type_check | |||
def __new__(cls, url: Optional[str], **kwargs) -> object: | |||
return str.__new__(cls, cls.build(**kwargs) if url is None else url) | |||
return str.__new__(cls, cls.build(**kwargs) if url is None else url) # noqa |
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 would remove all the # noqa
. I understand some IDEs like Pycharm will display some kind of warning but as long as flake8
says nothing we're good
Thanks a lot! 🙏 For the default port don't we want to add @staticmethod
def get_default_parts(parts: 'Parts') -> 'PartialParts':
return {
'port': '80' if parts['scheme'] == 'http' else '443',
} I thought this was wanted by @davidolrik in the other issue And don't worry about the CI 😉 It's due to the new sqlalchemy 1.4 version that is not pinned nor supported by |
Thank you for feedback!
Maybe use only UPD |
@PrettyWood 👻 🥳 |
Nevermind :) |
KafkaDsn
to networks.py
KafkaDsn
to networks.py
and add default ports for HttpUrl
Can you rebase please ? |
@PrettyWood done as you requested. All tests passed according to CI.
Also codecov is less then 100% and I am not sure why :c |
You have all the commits from master that unrelated. Please rebase on the upstream version and push force to have a clean change diff. And we'll see after for the coverage |
Looks like I broke everything in this branch( |
Lol no you didn't ;) it's just your rebase that was done on your local version of master and not the upstream one. I can rewrite your history if you need help |
I'd appreciate that! |
88e98ef
to
63fb064
Compare
All tests passed. That is great! |
Samuel merges the PRs. And a 1.8.2 may be released before 1.9. |
@PrettyWood ok, thanks! Did you forget to click 'Approve' button or there is something I should fix? |
5b99959
to
067e5ef
Compare
Thank you! |
…pydantic#2447) * added KafkaDsn to network * added short description to chandes folder * added default non-displayable ports to HttpUrl model * added info to changes folder * fix: support non default ports in HttpUrl * fix pr issues * remove noqa * add more typing by @PrettyWood * add default http and https ports to `HttpUrl` model * fix mypy * chore: do not add implementation details Co-authored-by: PrettyWood <em.jolibois@gmail.com>
❤️ pydantic
Change Summary
KafkaDsn
class e.g. already existingRedisDsn
🔥default_parts
member andapply_default_parts
method to theAnyUrl
classRelated issue number
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)