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.

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

Add AmqpDsn class #3254

Merged
merged 19 commits into from Dec 18, 2021
Merged

Add AmqpDsn class #3254

merged 19 commits into from Dec 18, 2021

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Sep 23, 2021

Change Summary

This PR is a continuation of #2049. I've respectfully maintained the commits, and fix the code according to @samuelcolvin previous comments.

I've also changed the name of the class RabbitmqDsn to RabbitMqDsn. As the "MQ" case should follow a similar analogy to "DSN".

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)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@Kludex Kludex marked this pull request as ready for review September 23, 2021 18:28
@Kludex
Copy link
Member Author

Kludex commented Sep 23, 2021

The image is from #2049 (review).

image

I don't understand what I'm supposed to add, as there's a description talking about amqp below. Would you mind clarifying?

@@ -21,6 +22,7 @@ class Settings(BaseSettings):

redis_dsn: RedisDsn = 'redis://user:pass@localhost:6379/1'
pg_dsn: PostgresDsn = 'postgres://user:pass@localhost:5432/foobar'
rabbitmq_dsn: RabbitMqDsn = 'amqp://guest:guest@rabbitmq:5672//'
Copy link
Member Author

Choose a reason for hiding this comment

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

/ is the default vhost. Should I explain something about it? If yes, where?

@Kludex
Copy link
Member Author

Kludex commented Sep 23, 2021

please review 😉

@nuno-andre
Copy link
Contributor

Hi @Kludex!

  • I think this class should be called AmqpDsn. AMQP is a standard and it's used by other brokers (StormMQ, ActiveMQ...). Also, RabbitMQ supports other protocols with different URI schemes (e.g. MQTT).
  • amqps should be added as an allowed scheme.
  • A username is not a requirement for a valid AMQP URI.

@Kludex
Copy link
Member Author

Kludex commented Sep 27, 2021

Whatever name suits people better is fine by me. I just want the type for AMPQ. But I can also include the other protocols supported for RabbitMQ.

For the two other comments, I'll address later on. Thank you!

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.

Will require quite a lot of renaming I'm afraid, otherwise LGTM.

docs/usage/types.md Show resolved Hide resolved
docs/usage/types.md Outdated Show resolved Hide resolved
pydantic/networks.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Member

please update.

@Kludex
Copy link
Member Author

Kludex commented Dec 12, 2021

please review

@github-actions github-actions bot assigned PrettyWood and samuelcolvin and unassigned Kludex Dec 12, 2021
@samuelcolvin samuelcolvin changed the title Add RabbitMqDsn class Add AmqpDsn class Dec 18, 2021
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.

otherwise LGTM, I'll make those two small changes.

changes/3254-kludex.md Outdated Show resolved Hide resolved
docs/examples/settings_main.py Outdated Show resolved Hide resolved
@samuelcolvin samuelcolvin merged commit 7eaa582 into pydantic:master Dec 18, 2021
@keyz182 keyz182 mentioned this pull request Nov 16, 2022
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants