added `KafkaDsn` to `networks.py` and add default ports for `HttpUrl` #2447

MihanixA · 2021-03-02T00:15:09Z

❤️ pydantic

Change Summary

Added KafkaDsn class e.g. already existing RedisDsn 🔥
Added default_parts member and apply_default_parts method to the AnyUrl class

Related issue number

Checklist

Unit tests for the changes exist
Tests pass on CI and coverage remains at 100%
Documentation reflects the changes where applicable
changes/<pull request or issue id>-<github username>.md file added describing change
(see changes/README.md for details)

codecov · 2021-03-02T00:17:11Z

Codecov Report

Merging #2447 (ba58ec7) into master (7b7e705) will decrease coverage by 0.03%.
The diff coverage is 94.87%.

❗ Current head ba58ec7 differs from pull request most recent head 067e5ef. Consider uploading reports for the commit 067e5ef to get more accurate results

@@             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

Impacted Files	Coverage Δ
pydantic/__init__.py	`100.00% <ø> (ø)`
pydantic/networks.py	`99.32% <94.87%> (-0.68%)`	⬇️

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 1a9f59d...067e5ef. Read the comment docs.

davidolrik · 2021-03-02T12:20:43Z

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?

MihanixA · 2021-03-02T13:05:39Z

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 KafkaDsn model. Default host and port are included to the str version.

https://github.com/samuelcolvin/pydantic/pull/2447/files#diff-f953eed7c7855cc439287af4864f147a1ed9c36995d18f335b57b96e3d7653b3R385

MihanixA · 2021-03-02T13:19:05Z

@davidolrik I could commit this solution to this PR and link your issue to this ticket. What do you think?

MihanixA · 2021-03-02T14:08:47Z

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.
@davidolrik @samuelcolvin

davidolrik · 2021-03-02T14:16:25Z

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'

davidolrik · 2021-03-02T14:19:52Z

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:

>       assert m.v.port == '1234'
E       AssertionError: assert '443' == '1234'
E         - 1234
E         + 443

MihanixA · 2021-03-02T14:26:53Z

I didn't notice any test that was not expecting to get a ValidationError in test_networks.py with ports other than default (443 and 80).
So for moment I decided that other ports are not expected in this model.

I had already been working on fix by the time I read your message though
But thank you any way! 😄

MihanixA · 2021-03-02T16:05:34Z

@davidolrik added tests as you requested

MihanixA · 2021-03-02T16:45:37Z

@samuelcolvin ready for review

MihanixA · 2021-03-03T19:52:37Z

@PrettyWood I'd appreciate review and/or some feedback 🛩️ 🙏

changes/2447-MihanixA.md

pydantic/networks.py

MihanixA · 2021-03-16T17:16:12Z

I couldn't make this work with default port in HttpUrl for now. I really tried though
So I guess I am going to commit fixes for the PR issues and try to solve HttpUrl problem in a separate PR..

MihanixA · 2021-03-16T17:56:12Z

I added some # noqa (pls tell if you want them removed)
And also made HttpUrl inherit AnyHttpUrl instead of AnyUrl

MihanixA · 2021-03-16T18:03:59Z

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..
UPD noticed that other PR's fail at the same test

MihanixA · 2021-03-20T18:04:13Z

@PrettyWood 🙃 🎉

PrettyWood · 2021-03-24T10:34:57Z

pydantic/networks.py

@@ -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


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

PrettyWood · 2021-03-24T10:45:24Z

Thanks a lot! 🙏
I guess we could add some type hints for the parts with TypedDict!
I didn't want to commit on your branch so here is a patch for inspiration. You're welcome to use it or not!

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 encoded/databases. I opened encode/databases#299

MihanixA · 2021-03-24T14:45:50Z

Thank you for feedback!

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',
        }

Maybe use only 'PartialParts' without 'Parts'? By the time get_default_parts is called, parts argument doesn't comply with described TypedDict. Besides this move would allow to avoid code duplication.

UPD
About the issue with default http port: it worked before get_default_parts became a @staticmethod. Now I don't see a clear solution. I'd like to continue to work on that issue in a separate PR when the situation about default parts is finalized)

MihanixA · 2021-03-25T15:03:46Z

@PrettyWood 👻 🥳

MihanixA · 2021-03-25T21:07:58Z

I'd like to continue to work on that issue in a separate PR when the situation about default parts is finalized)

Nevermind :)

PrettyWood · 2021-04-05T10:34:25Z

Can you rebase please ?

MihanixA · 2021-04-05T13:48:07Z

@PrettyWood done as you requested.

All tests passed according to CI.
Though when I run tests locally, I get this:

========================================================================================= short test summary info ==========================================================================================
FAILED tests/test_generics.py::test_replace_types - IndexError: tuple index out of range
FAILED tests/test_schema.py::test_unenforced_constraints_schema[kwargs9-type_9] - IndexError: tuple index out of range

Results (57.85s):
    2045 passed
       2 failed
         - tests/test_generics.py:787 test_replace_types
         - tests/test_schema.py:1442 test_unenforced_constraints_schema[kwargs9-type_9]
       6 skipped
make: *** [test] Error 1

Also codecov is less then 100% and I am not sure why :c

PrettyWood · 2021-04-05T13:50:10Z

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

MihanixA · 2021-04-05T15:17:00Z

Looks like I broke everything in this branch(
I am going to close this PR and create a new one with clear change history
OK? @PrettyWood

PrettyWood · 2021-04-05T16:21:27Z

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

MihanixA · 2021-04-05T16:27:22Z

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!

MihanixA · 2021-04-12T12:53:29Z

All tests passed. That is great!
The only thing that remains is to push merge button 👻

MihanixA · 2021-04-21T12:28:22Z

@PrettyWood

PrettyWood · 2021-04-21T12:45:31Z

Samuel merges the PRs. And a 1.8.2 may be released before 1.9.

MihanixA · 2021-05-03T13:21:42Z

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?

PrettyWood · 2021-09-03T23:33:38Z

Thank you!

@PrettyWood

…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>

MihanixA changed the title ~~added KafkaDsn to network~~ added KafkaDsn to networks.py Mar 2, 2021

PrettyWood reviewed Mar 3, 2021

View reviewed changes

changes/2447-MihanixA.md Outdated Show resolved Hide resolved

pydantic/networks.py Outdated Show resolved Hide resolved

pydantic/networks.py Outdated Show resolved Hide resolved

pydantic/networks.py Outdated Show resolved Hide resolved

PrettyWood added the awaiting author revision label Mar 15, 2021

MihanixA requested a review from PrettyWood March 16, 2021 17:59

MihanixA changed the title ~~added KafkaDsn to networks.py~~ added KafkaDsn to networks.py Mar 19, 2021

PrettyWood reviewed Mar 24, 2021

View reviewed changes

MihanixA changed the title ~~added KafkaDsn to networks.py~~ added KafkaDsn to networks.py and add default ports for HttpUrl Mar 26, 2021

MihanixA force-pushed the add-kafka-dsn branch from c988e04 to ba58ec7 Compare March 26, 2021 19:08

MihanixA requested a review from PrettyWood April 3, 2021 11:25

MihanixA force-pushed the add-kafka-dsn branch from 88337a3 to 88e98ef Compare April 5, 2021 15:06

MihanixA and others added 10 commits April 5, 2021 22:58

added KafkaDsn to network

ba0dc20

added short description to chandes folder

94112a5

added default non-displayable ports to HttpUrl model

b591c11

added info to changes folder

3aea716

fix: support non default ports in HttpUrl

001814e

fix pr issues

7ff629c

remove noqa

31e3ebe

add more typing by @PrettyWood

bd752fc

add default http and https ports to HttpUrl model

cd8b645

fix mypy

63fb064

PrettyWood force-pushed the add-kafka-dsn branch from 88e98ef to 63fb064 Compare April 5, 2021 21:09

PrettyWood added 2 commits September 4, 2021 01:20

Merge branch 'master' into add-kafka-dsn

e7eb388

chore: do not add implementation details

067e5ef

PrettyWood force-pushed the add-kafka-dsn branch from 5b99959 to 067e5ef Compare September 3, 2021 23:33

PrettyWood approved these changes Sep 3, 2021

View reviewed changes

PrettyWood removed the awaiting author revision label Sep 3, 2021

PrettyWood merged commit 65fc336 into pydantic:master Sep 3, 2021

PrettyWood mentioned this pull request Sep 4, 2021

fix: coverage #3169

Merged

5 tasks

dolfinus mentioned this pull request Jan 10, 2022

Fix missing port in HttpUrl.build() result #3652

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added `KafkaDsn` to `networks.py` and add default ports for `HttpUrl` #2447

added `KafkaDsn` to `networks.py` and add default ports for `HttpUrl` #2447

MihanixA commented Mar 2, 2021 •

edited

codecov bot commented Mar 2, 2021 •

edited

davidolrik commented Mar 2, 2021

MihanixA commented Mar 2, 2021 •

edited

MihanixA commented Mar 2, 2021 •

edited

MihanixA commented Mar 2, 2021

davidolrik commented Mar 2, 2021

davidolrik commented Mar 2, 2021

MihanixA commented Mar 2, 2021 •

edited

MihanixA commented Mar 2, 2021

MihanixA commented Mar 2, 2021

MihanixA commented Mar 3, 2021 •

edited

MihanixA commented Mar 16, 2021

MihanixA commented Mar 16, 2021

MihanixA commented Mar 16, 2021 •

edited

MihanixA commented Mar 20, 2021

PrettyWood Mar 24, 2021 •

edited

PrettyWood commented Mar 24, 2021 •

edited

MihanixA commented Mar 24, 2021 •

edited

MihanixA commented Mar 25, 2021

MihanixA commented Mar 25, 2021

PrettyWood commented Apr 5, 2021

MihanixA commented Apr 5, 2021

PrettyWood commented Apr 5, 2021

MihanixA commented Apr 5, 2021 •

edited

PrettyWood commented Apr 5, 2021

MihanixA commented Apr 5, 2021

MihanixA commented Apr 12, 2021

MihanixA commented Apr 21, 2021

PrettyWood commented Apr 21, 2021

MihanixA commented May 3, 2021 •

edited

PrettyWood commented Sep 3, 2021

added KafkaDsn to networks.py and add default ports for HttpUrl #2447

added KafkaDsn to networks.py and add default ports for HttpUrl #2447

Conversation

MihanixA commented Mar 2, 2021 • edited

Change Summary

Related issue number

Checklist

codecov bot commented Mar 2, 2021 • edited

Codecov Report

davidolrik commented Mar 2, 2021

MihanixA commented Mar 2, 2021 • edited

MihanixA commented Mar 2, 2021 • edited

MihanixA commented Mar 2, 2021

davidolrik commented Mar 2, 2021

davidolrik commented Mar 2, 2021

MihanixA commented Mar 2, 2021 • edited

MihanixA commented Mar 2, 2021

MihanixA commented Mar 2, 2021

MihanixA commented Mar 3, 2021 • edited

MihanixA commented Mar 16, 2021

MihanixA commented Mar 16, 2021

MihanixA commented Mar 16, 2021 • edited

MihanixA commented Mar 20, 2021

PrettyWood Mar 24, 2021 • edited

Choose a reason for hiding this comment

PrettyWood commented Mar 24, 2021 • edited

MihanixA commented Mar 24, 2021 • edited

MihanixA commented Mar 25, 2021

MihanixA commented Mar 25, 2021

PrettyWood commented Apr 5, 2021

MihanixA commented Apr 5, 2021

PrettyWood commented Apr 5, 2021

MihanixA commented Apr 5, 2021 • edited

PrettyWood commented Apr 5, 2021

MihanixA commented Apr 5, 2021

MihanixA commented Apr 12, 2021

MihanixA commented Apr 21, 2021

PrettyWood commented Apr 21, 2021

MihanixA commented May 3, 2021 • edited

PrettyWood commented Sep 3, 2021

added `KafkaDsn` to `networks.py` and add default ports for `HttpUrl` #2447

added `KafkaDsn` to `networks.py` and add default ports for `HttpUrl` #2447

MihanixA commented Mar 2, 2021 •

edited

codecov bot commented Mar 2, 2021 •

edited

MihanixA commented Mar 2, 2021 •

edited

MihanixA commented Mar 2, 2021 •

edited

MihanixA commented Mar 2, 2021 •

edited

MihanixA commented Mar 3, 2021 •

edited

MihanixA commented Mar 16, 2021 •

edited

PrettyWood Mar 24, 2021 •

edited

PrettyWood commented Mar 24, 2021 •

edited

MihanixA commented Mar 24, 2021 •

edited

MihanixA commented Apr 5, 2021 •

edited

MihanixA commented May 3, 2021 •

edited