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

By default, the virtual host configured in the uri is used #238

Closed
wants to merge 1 commit into from
Closed

By default, the virtual host configured in the uri is used #238

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions _examples/producer/producer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@ package main
import (
"context"
"flag"
amqp "github.com/rabbitmq/amqp091-go"
"log"
"os"
"os/signal"
"syscall"
"time"

amqp "github.com/rabbitmq/amqp091-go"
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change due to your preference, or to a tool auto-formatting the code?

Copy link
Author

Choose a reason for hiding this comment

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

by go fmt

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there other example code that should be formatted as part of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

go fmt ./... doesn't make this change for me 😕 Are you running go fmt with any special flag?

Copy link
Author

Choose a reason for hiding this comment

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

go fmt ./... doesn't make this change for me 😕 Are you running go fmt with any special flag?

Yes, you are right, I think my submitted code was automatically formatted by the development tool (goland); I hope you ignore the submission about here. Thanks

)

var (
Expand Down Expand Up @@ -58,7 +59,6 @@ func setupCloseHandler(exitCh chan struct{}) {

func publish(ctx context.Context, publishOkCh <-chan struct{}, confirmsCh chan<- *amqp.DeferredConfirmation, confirmsDoneCh <-chan struct{}, exitCh chan struct{}) {
config := amqp.Config{
Vhost: "/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does other example code use Vhost: when it's not necessary?

Copy link
Author

Choose a reason for hiding this comment

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

My suggestion is that the configured uri vhost should be used first. In other cases, you can configure it yourself.
Of course it's just my suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

#238 (comment)

I'm not sure I understand the benefit of this. IMO, having Vhost field set in the config is very clear, and it's better than relying in the implicit behaviour of URI parsing.

Copy link
Author

Choose a reason for hiding this comment

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

#238 (comment)

I'm not sure I understand the benefit of this. IMO, having Vhost field set in the config is very clear, and it's better than relying in the implicit behaviour of URI parsing.

The reason why I modified it like this is because after I set the Vhost in the uri, I ran the code and reported an error. After comparison, I found that this was not the vhost I set.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe this is just my personal understanding. If you think the original way is better, I think my submission can be canceled. At the same time, I am also happy to communicate with you, thank you.

Properties: amqp.NewConnectionProperties(),
}
config.Properties.SetClientConnectionName("producer-with-confirms")
Expand Down