Reviewers guideline for Pull Request

[MovieStoreGuy]

Having reviewed a few pull requests, the quality and the approach on how things are done vary a lot and it raised some questions that I wanted to see if we can clarify to help making the PR process easier.

There is a few things I wanted to highlight as such:

• Added code is not go idiomatic (lack of concise variables, naming conventions, stuttering within definitions)
• Mocking external integrations
• The suggest packages to use for testing
• Generated data for testing
• Test helper functions

There is rather extreme variations in the work of the pull requests and I would like to make the reviewing experience consistent regardless of the reviewer.

[MovieStoreGuy]

@bogdandrutu , @jpkrohling , @codeboten, @tigrannajaryan,

Could I ask for your input on this as the maintainers of the project 🥺 ?

[jpkrohling]

It would be great to have a review guideline! The review guidelines would help not only reviewers but also what contributors can expect from us.

I would add to your list which library to use for hashing (see #6136).

[tigrannajaryan]

• Added code is not go idiomatic (lack of concise variables, naming conventions, stuttering within definitions)

We have this https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#coding-guidelines recommendation which reffers to https://golang.org/doc/effective_go.html, perhaps we need to explain more.

[MovieStoreGuy]

I did miss that @tigrannajaryan, thanks for sharing.

Perhaps it would be worth extending it to include what is the suggestion in order to do X and contribute that back to CONTRIBUTING.md ?

I think it would go some way when reviewing and help ask better clarification.

Is that a sensible way to go about it?

[MovieStoreGuy]

I should also add that requiring CGO should be made clear as part of the package.

[tigrannajaryan]

Perhaps it would be worth extending it to include what is the suggestion in order to do X and contribute that back to CONTRIBUTING.md ?

Definitely worth extending.

[MovieStoreGuy]

@tigrannajaryan if you wanna assign this to me, I'll have a PR to iterate over by the end of the day.

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

[jpkrohling]

I'm closing this, as I understand the initial proposal was completed.