Unidiomatic module layout and examples

[somebadcode]

Is your feature request related to a problem? Please describe.

The project is too nested and since it's only a package that's intended to be imported by anyone who wants to interact with RabbitMQ in Go, the core functionality should be in the root of the module.

So when you want to use this module, you simply import github.com/rabbitmq/rabbitmq-amqp-go-client instead of github.com/rabbitmq/rabbitmq-amqp-go-client/pkg/rabbitmqamqp.

Describe the solution you'd like

I recommend that you flatten it so that it's more idiomatic and easier to read for anyone who isn't a maintainer.

The examples that currently reside in docs/examples, should instead be written as examples — as documented in the Go documentation.

Examples are explained here: https://go.dev/blog/examples

The benefit of writing examples as shown by the Go blog article is that they show up as examples in the documentation generated from your code.

So instead of users having to go to the git repository, they can directly see the examples in the generated documentation

So if you flatten the project and name the package like this:

package rabbitmqamqp

Then test files that exclusively test exported functionality as well as example code should be in _test.go files that have the package name suffix _test, e.g

Filename example tls_example_test.go:

package rabbitmqamqp_test

func ExampleTLS() {
	// to run the example you can use the certificates from the rabbitmq-amqp-go-client
	// inside the directory .ci/certs

	caCert, err := os.ReadFile("/path/ca_certificate.pem")
	if err != nil {
		panic(err) // handling omitted for brevity.
	}

	// Create a CA certificate pool and add the CA certificate to it
	caCertPool := x509.NewCertPool()
	caCertPool.AppendCertsFromPEM(caCert)

	// Load client cert
	clientCert, err := tls.LoadX509KeyPair("/path//client_localhost_certificate.pem", "/path//client_localhost_key.pem")
	if err != nil {
		panic(err) // handling omitted for brevity.
	}

	// Create a TLS configuration
	tlsConfig := &tls.Config{
		Certificates:       []tls.Certificate{clientCert},
		RootCAs:            caCertPool,
		InsecureSkipVerify: false,
		ServerName:         "localhost", // the server name should match the name on the certificate
	}

	env := rmq.NewClusterEnvironment([]rmq.Endpoint{
		{
			Address: "amqps://localhost:5671",
			Options: &rmq.AmqpConnOptions{
				SASLType:  amqp.SASLTypeAnonymous(),
				TLSConfig: tlsConfig,
			},
		},
	})

	connection, err := env.NewConnection(context.TODO())
	if err != nil {
		panic(err) // handling omitted for brevity.
	}

	// Close the connection
	connection.Close(context.TODO())
	if err != nil {
		panic(err) // handling omitted for brevity.
	}
}

This code will become part of the generated documentation. It doesn't have to be a runnable example. Examples that contain the special output comment syntax can be used as tests as well, and become runnable directly on the documentation page and are automatically run by go test.

Key points:

1. Put the primary functionality up front in the root of the module (git repo) and only put extra optional features that are you want to separate from the package in the module root into sub-packages (not nested inside pkg/).
2. Make examples part of the generated documentation that gets published on go.dev so that they are next to the actual documentation.
3. The folder name pkg has no meaning and only adds to the noise of the module. It's a bad habit brought by others that come from other languages, into Go projects and it was never part of any official documentation or recommendation.
4. Hide any form of helpers that you don't want others to import as a sub-package inside internal/ such as internal/test-helper. This makes it so that only this project can use it and you're free to make any breaking changes to it since no other projects can import it.
5. Helpers used inside tests should accept t *testing.T as its first argument and t.Helper() should be called before any code is executed in said helpers. Helpers can fail the tests if something goes wrong.
6. Do not use context.Background() in examples or tests. Examples should use context.TODO() to indicate that it should be something that the developer has to provide and not the background context. Unit tests that use context should use t.Context().
7. Unit tests should be actual functions that contain 1 or more test cases. Runnable examples are great for simple tests that make assertions as well as show functionality. Otherwise, table driven tests are the recommended approach.
8. Do not try to replicate the design patterns and naming conventions used in other languages. I don't bring Go patterns to Java or Python for example. You get the cleanest and most user friendly code if you write code that's tailored to the target audience and ecosystem.

It's worth making these breaking changes when the module is still young and hasn't reached a major release yet.

Describe alternatives you've considered

No response

Additional context

No response

[Gsantomaggio]

Thank you @somebadcode.
It makes sense to me, but let's hear what others think. cc @Zerpet

@hiimjako WDYT? (Even if you don't follow this project :))

[hiimjako]

@hiimjako WDYT? (Even if you don't follow this project :))

I agree with all the suggestions :)
It would make sense to apply the non-breaking suggestions to the client stream as well, such as documentation with examples and tests.

[Zerpet]

3. The folder name pkg has no meaning and only adds to the noise of the module. It's a bad habit brought by others that come from other languages, into Go projects and it was never part of any official documentation or recommendation.

Yep, bad habits by others like Kubernetes project.

5. Helpers used inside tests should accept t *testing.T as its first argument and t.Helper() should be called before any code is executed in said helpers. Helpers can fail the tests if something goes wrong.
6. Do not use context.Background() in examples or tests. Examples should use context.TODO() to indicate that it should be something that the developer has to provide and not the background context. Unit tests that use context should use t.Context().
7. Unit tests should be actual functions that contain 1 or more test cases. Runnable examples are great for simple tests that make assertions as well as show functionality. Otherwise, table driven tests are the recommended approach.

AI fluff not applicable to this project since we use Ginkgo and Gomega.

That being said, there's some good feedback we can take in. For example, moving test helpers and other exported types to internal package. Moving the example to a ExampleXYZ function is also sensible, since most of our examples are one file. However, we should put build tags on the examples, otherwise, ginkgo runner will always run the examples, which can add a lot of noise when you are debugging a test.

Regarding the main point of moving all the source files to the root of the repo, I don't have a strong opinion. Projects like Kubernetes don't do that, our cluster operator for kubernetes doesn't do that either, so it's really just a matter of taste. Arguing that adding 17 extra characters to an import line creates noise is a bit dramatic.

[somebadcode]

3. The folder name pkg has no meaning and only adds to the noise of the module. It's a bad habit brought by others that come from other languages, into Go projects and it was never part of any official documentation or recommendation.

Yep, bad habits by others like Kubernetes project.

5. Helpers used inside tests should accept t *testing.T as its first argument and t.Helper() should be called before any code is executed in said helpers. Helpers can fail the tests if something goes wrong.
6. Do not use context.Background() in examples or tests. Examples should use context.TODO() to indicate that it should be something that the developer has to provide and not the background context. Unit tests that use context should use t.Context().
7. Unit tests should be actual functions that contain 1 or more test cases. Runnable examples are great for simple tests that make assertions as well as show functionality. Otherwise, table driven tests are the recommended approach.

AI fluff not applicable to this project since we use Ginkgo and Gomega.

That being said, there's some good feedback we can take in. For example, moving test helpers and other exported types to internal package. Moving the example to a ExampleXYZ function is also sensible, since most of our examples are one file. However, we should put build tags on the examples, otherwise, ginkgo runner will always run the examples, which can add a lot of noise when you are debugging a test.

Regarding the main point of moving all the source files to the root of the repo, I don't have a strong opinion. Projects like Kubernetes don't do that, our cluster operator for kubernetes doesn't do that either, so it's really just a matter of taste. Arguing that adding 17 extra characters to an import line creates noise is a bit dramatic.

I wrote all of this myself and I haven't used an LLM in the process at all. I read the code myself and wrote the recommendations all by myself. It sounds dramatic to call this AI fluff when no such thing was part of the process.

I'm pointing out code smells. Unnecessary nesting is not only "17 extra characters".

If ginkgo will run your examples all the time and you end up having to use build tags to prevent it then ginkgo is not recommended. It's not something I would ever add to my own projects and would argue against it at work.

What does ginkgo offer that testing doesn't and does it warrant a sidestep from writing idiomatic go? I'm interested in the choice of ginkgo over using testing.

[somebadcode]

3. The folder name pkg has no meaning and only adds to the noise of the module. It's a bad habit brought by others that come from other languages, into Go projects and it was never part of any official documentation or recommendation.

Yep, bad habits by others like Kubernetes project.

Kubernetes is a bad example of how to write Go code.

More on layout: https://go.dev/doc/modules/layout
You have a flatter layout here: https://github.com/rabbitmq/amqp091-go

And if you want to read more about writing idiomatic Go, Google has a set of guidelines that I find useful: https://google.github.io/styleguide/go/index

The use of ginkgo looks like you're bringing rspec into Go.

[Zerpet]

I wrote all of this myself and I haven't used an LLM in the process at all. I read the code myself and wrote the recommendations all by myself. It sounds dramatic to call this AI fluff when no such thing was part of the process.

In the OP there are mentions and recommendations to testing package, which is not in use at all since Ginkgo is our chosen test framework. That's why it smelled to me that some of the OP points were AI fluff. Kudos to you for writing all this feedback.

If ginkgo will run your examples all the time and you end up having to use build tags to prevent it then ginkgo is not recommended. It's not something I would ever add to my own projects and would argue against it at work.

That's your opinion. My opinion is that Ginkgo, in conjunction with Gomega, provides a much better experience for writing and running tests in Go. We don't have to agree, nor try to convince each other. For certain, I'm not going to rewrite our entire test code with testing just because "Ginkgo is not idiomatic".

What does ginkgo offer that testing doesn't and does it warrant a sidestep from writing idiomatic go? I'm interested in the choice of ginkgo over using testing.

Onsi did a very good job at documenting why ginkgo: https://onsi.github.io/ginkgo/#why-ginkgo

You have a flatter layout here: https://github.com/rabbitmq/amqp091-go

True, although not by choice, but inheritance. You have more details in amqp091-go readme.

Kubernetes is a bad example of how to write Go code.

Sure, consider myself and Kubernetes declared Go apostates. If you have specific, concrete and actionable recommendations for specific pieces of code in this client, you're feedback is most welcome. I'm not keen on moving files around and dropping our good trusted Ginkgo for the sake of "idiomatic" Go.