docs: Add coding-conventions.md

[0xFelix]

What this PR does

This adds coding-convetions.md which contains coding guidelines for the KubeVirt project.

Fixes #11259

Why we need it and why it was done in this way

We need coding conventions to ensure the maintainability of the codebase.

The following tradeoffs were made:

The rules are opinionated and subject to discussion/change.

The following alternatives were considered:

Do not provide guidelines and become desperate maintaining the code. :)

Links to places where the discussion took place:

See #11259

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

• Design: A design document was considered and is present (link) or not required
• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
• Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
• Community: Announcement to kubevirt-dev was considered

Release note

None

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign alonakaplan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[0xFelix]

/sig code-quality

[lyarwood]

#11446 (comment)

Could we allow this when delegating the return to another function as a way to document the return variables?

[EdDev]

This is actually enforced when appropriate by the current linter (if the code path is covered by it).

Documentation is needed when you have two returned values (or more) of the same type, to provide the documentation.
I think there is no other need for documentation when the function is simple enough and well named.
If the function is not well named and there are too many returned values, it is usually a hint of a problem and that should be treated instead.
But obviously there are cases when it makes total sense to return two strings and an error (as an example) and in those cases naming is good.

The main takeaway IMO from this should be: It is an exception to need it and one should try to avoid it because it is a smell.

[lyarwood]

Is this a suggestion from k8s?

We have PatchOperation in our tree that I've used and wanted to advocate for in the past over fmt.Sprintf when building patches.

[alicefr]

@lyarwood this would make more sense when this PR is merged: #11211 .But, it is an extension of the PatchOperation

[iholder101]

Kudos @0xFelix!
Great seeing this :)

[iholder101]

The link redirects you to https://go.dev/wiki/CodeReviewComments

[0xFelix]

Thanks, will fix it

[iholder101]

While I think it's great to keep these as references here, I think that these go a bit beyond "conventions", but are also talk about "correct code" in general.

Do you think it makes sense to just link these as a reference outside the list of conventions?

[0xFelix]

We could drop them of course, I added the from the initial Kubernetes coding-conventions.md, but I don't see them as a hard requirement as you say.

[iholder101]

I know I'm on the minority on this one, but I'll open it here to debate since maybe someone would agree 😄

It's a matter of style and taste I guess, but when I look at:

err := functionThatDoesSomething(param1, param2, param3)
Expect(err).ToNot(HaveOccurred())

I can clearly see what's being executed and what's the validation being made.
While here:

Expect(functionThatDoesSomething(param1, param2, param3)).To(Succeed())

It looks like a big mess. A long one-liner that kind of hides the fact that we do an operation + validation at the same time.

As said I know I'm the minority here and probably will lose this battle :)

[machadovilaca]

While in the specific ginkgo case I tend to agree with you, because you can additionally have the error descriptions in the Expect to further complicate the line, I can see the benefits of inlining the error in the following situations:

err := functionThatDoesSomething(param1, param2, param3)
if err != nil {
  return err
}

That could be rewritten as:

if err := functionThatDoesSomething(param1, param2, param3); err != nil
  return err
}

[EdDev]

Expect(functionThatDoesSomething(param1, param2, param3)).To(Succeed())

It looks like a big mess. A long one-liner that kind of hides the fact that we do an operation + validation at the same time.

From a human language point of view, Expect(RunSomething(a,b,c)).To(Succeed()) read better and focuses on the success of the call.
Furthermore, it supports functions that return a return value and an error, fitting cases when you do not care about the value (e.g. when creating a VMI object), you just care about its success.

[0xFelix]

@iholder101 Yes, I agree with that. It was meant for cases like @machadovilaca depicted.

@EdDev Does it really support functions returning multiple values (values+error)? From the docs it does not sound like that:

You should not use a function with multiple return values (like DoSomethingHard) with Succeed

See https://onsi.github.io/gomega/#handling-errors

[iholder101]

I strongly disagree.
Let's continue here #11446

[EdDev]

Makes sense to continue here, as that specific issue is one instance of many possible ones.

I would rephrase it a bit to express the problem.
E.g.

Use named return values when a good reason exists. Reason for it.

Then examples may be given for when it is a fit.
There are actually only a handful of reasons to use them and it is a good practice to see these uses as exceptions.

[0xFelix]

I agree, we should not forbid them completely but make authors think twice before using them. Can we gather a list of good reasons to use them here?

[iholder101]

Another useful use case of closures is to define a function that is relevant only for a very specific scope.
Since go doesn't have private variables and every function is shared with the whole package this is sometimes important IMO

[dankenigsberg]

The solution for a possible in-package conflict is to ensure that packages have a maintainable size (not too many files, not too long files) and that functions have clear useful names.

Defining a function inside another is a liability as

• it makes the caller function longer
• it forces a reader of the caller function to go through implementation details
• it forces a reader to wonder if the internal function is ever going to be replaced during runtime, adding to cognitive load.
• it risks capturing unintended variable from within the scope of the definition of the function
• it risks causing side effects on internal variables of the caller function

[iholder101]

The solution for a possible in-package conflict is to ensure that packages have a maintainable size (not too many files, not too long files) and that functions have clear useful names.

I agree.
However, sometimes even with small package sizes a helper function is relevant only for a specific scope.

I have two examples in mind for these cases.

The first: Let's say that a function needs to do an operation for a few times. Let's say that this operation is specific to the function's operation and has no meaning outside of it. In this scenario it makes total sense to wrap this helper operation within a variable-function and use it within the parent function when needed. The alternative of extracting the function is both error-prone (as someone might use it) and confusing (since someone might spend time on reasoning about this function in a scope where it doesn't make sense).

The second: tests. Usually there's more than one test in a package, and with unit tests it's almost a must since the test files reside in the production file-tree. It's very common that a very weird operation would be needed in order to test something, but that it wouldn't have any meaning outside the specific test's context.

---

I guess that my problem is with the broad statement that variable functions are a bad thing. Yes, sometimes they are, but sometimes they do make sense. When making such a policy that would later be enforced in code reviews we need to be careful from such broad statements.

[iholder101]

I would suggest:

Suggested change

	- Avoid closures when they do not capture any meaningful state
	- Use closures with caution, be aware of the risks and use them only when it makes sense.

[dankenigsberg]

The text of this page should state that maintainers are allowed to make well-motivated exceptions. But each convention should be a broad opinionated statement. I like @fabiand 's idea of adding examples for acceptable code and unacceptable one. @iholder101 I'm certain you can provide a good use for variable functions. I can provide several problematic ones in https://github.com/kubevirt/kubevirt/blob/247ee51/tests/operator/operator.go#L125

[iholder101]

Challenge accepted!
I'll also try to fetch code that I have specifically wrote.

---

For my first example above

Here's a function that generates the error and being used multiple times in the parent function. It must be used only within this scope and has no meaning outside of it:

kubevirt/pkg/virtctl/create/clone/clone.go

Lines 261 to 299 in aa5a345

	func (c *createClone) typeToTypedLocalObjectReference(sourceOrTargetType, sourceOrTargetName string, isSource bool) (*v1.TypedLocalObjectReference, error) {
	var kind, apiGroup string
	generateErr := func() error {
	var sourceTargetStr string
	var supportedTypes string
	if isSource {
	sourceTargetStr = "source"
	supportedTypes = supportedSourceTypes
	} else {
	sourceTargetStr = "target"
	supportedTypes = supportedTargetTypes
	}
	return fmt.Errorf("%s type %s is not supported. Supported types: %s", sourceTargetStr, sourceOrTargetType, supportedTypes)
	}
	switch sourceOrTargetType {
	case "vm", "VM", "VirtualMachine", "virtualmachine":
	kind = "VirtualMachine"
	apiGroup = "kubevirt.io"
	case "snapshot", "VirtualMachineSnapshot", "vmsnapshot", "VMSnapshot":
	if !isSource {
	return nil, generateErr()
	}
	kind = "VirtualMachineSnapshot"
	apiGroup = "snapshot.kubevirt.io"
	default:
	return nil, generateErr()
	}
	return &v1.TypedLocalObjectReference{
	Name: sourceOrTargetName,
	Kind: kind,
	APIGroup: &apiGroup,
	}, nil
	}

Another example:

kubevirt/pkg/virt-handler/container-disk/mount.go

Lines 554 to 623 in aa5a345

	func (m *mounter) unmountKernelArtifacts(vmi *v1.VirtualMachineInstance) error {
	if !util.HasKernelBootContainerImage(vmi) {
	return nil
	}
	log.DefaultLogger().Object(vmi).Infof("unmounting kernel artifacts")
	kb := vmi.Spec.Domain.Firmware.KernelBoot.Container
	record, err := m.getMountTargetRecord(vmi)
	if err != nil {
	return fmt.Errorf("failed to get mount target record: %v", err)
	} else if record == nil {
	log.DefaultLogger().Object(vmi).Warning("Cannot find kernel-boot entries to unmount")
	return nil
	}
	unmount := func(targetDir *safepath.Path, artifactPaths ...string) error {
	for _, artifactPath := range artifactPaths {
	if artifactPath == "" {
	continue
	}
	targetPath, err := safepath.JoinNoFollow(targetDir, filepath.Base(artifactPath))
	if err != nil {
	return fmt.Errorf(failedCheckMountPointFmt, targetPath, err)
	}
	if mounted, err := isolation.IsMounted(targetPath); err != nil {
	return fmt.Errorf(failedCheckMountPointFmt, targetPath, err)
	} else if mounted {
	log.DefaultLogger().Object(vmi).Infof("unmounting container disk at targetDir %s", targetPath)
	out, err := virt_chroot.UmountChroot(targetPath).CombinedOutput()
	if err != nil {
	return fmt.Errorf(failedUnmountFmt, targetPath, string(out), err)
	}
	}
	}
	return nil
	}
	for idx, entry := range record.MountTargetEntries {
	if !strings.Contains(entry.TargetFile, containerdisk.KernelBootName) {
	continue
	}
	targetDir, err := safepath.NewFileNoFollow(entry.TargetFile)
	if err != nil {
	return fmt.Errorf("failed to obtaining a reference to the target directory %q: %v", targetDir, err)
	}
	_ = targetDir.Close()
	log.DefaultLogger().Object(vmi).Infof("unmounting kernel artifacts in path: %v", targetDir)
	if err = unmount(targetDir.Path(), kb.InitrdPath, kb.KernelPath); err != nil {
	// Not returning here since even if unmount wasn't successful it's better to keep
	// cleaning the mounted files.
	log.Log.Object(vmi).Reason(err).Error("unable to unmount kernel artifacts")
	}
	removeSliceElement := func(s []vmiMountTargetEntry, idxToRemove int) []vmiMountTargetEntry {
	// removes slice element efficiently
	s[idxToRemove] = s[len(s)-1]
	return s[:len(s)-1]
	}
	record.MountTargetEntries = removeSliceElement(record.MountTargetEntries, idx)
	return nil
	}
	return fmt.Errorf("kernel artifacts record wasn't found")
	}

---

For my second example above

There are many many examples, but here's one. The helper function is relevant for a specific test. Extracting it would make it harder to use and would bring no value to anyone outside the test:

kubevirt/tests/migration/migration.go

Lines 1212 to 1230 in aa5a345

	Context("with a Fedora shared NFS PVC (using nfs ipv4 address), cloud init and service account", func() {
	var vmi *v1.VirtualMachineInstance
	var dv *cdiv1.DataVolume
	var storageClass string
	createDV := func(namespace string) {
	url := "docker://" + cd.ContainerDiskFor(cd.ContainerDiskFedoraTestTooling)
	dv = libdv.NewDataVolume(
	libdv.WithRegistryURLSourceAndPullMethod(url, cdiv1.RegistryPullNode),
	libdv.WithPVC(
	libdv.PVCWithStorageClass(storageClass),
	libdv.PVCWithVolumeSize(cd.FedoraVolumeSize),
	libdv.PVCWithReadWriteManyAccessMode(),
	),
	)
	dv, err = virtClient.CdiClient().CdiV1beta1().DataVolumes(testsuite.GetTestNamespace(nil)).Create(context.Background(), dv, metav1.CreateOptions{})
	Expect(err).ToNot(HaveOccurred())
	}

---

Another example is for documentation's sake. As I've explained in detail here, I'm not a fan of comments. Sometimes it's better to define a one-time function just to give it a name and avoid using comments. An example from the above code is this:

 		removeSliceElement := func(s []vmiMountTargetEntry, idxToRemove int) []vmiMountTargetEntry { 
 			s[idxToRemove] = s[len(s)-1] 
 			return s[:len(s)-1] 
 		} 
  
 		record.MountTargetEntries = removeSliceElement(record.MountTargetEntries, idx) 
 		return nil 

I think this is better than:

		// removes slice element efficiently 
		record.MountTargetEntries[idx] = record.MountTargetEntries[len(s)-1] 
		record.MountTargetEntries = record.MountTargetEntries[:len(s)-1] 
 		return nil 

(note: there is an unnecessary comment in the actual code as well. It's a mistake IMO).

---

Another valuable use is to use defers:

kubevirt/cmd/virt-launcher-monitor/virt-launcher-monitor.go

Lines 424 to 450 in aa5a345

	func dumpLogFile(filePath string) {
	f, err := os.Open(filePath)
	if err != nil {
	if !errors.Is(err, os.ErrNotExist) {
	log.Log.Reason(err).Errorf("failed to open file %s", filePath)
	return
	}
	return
	}
	defer func() {
	if err := f.Close(); err != nil {
	log.Log.Reason(err).Errorf("failed to close file: %s", filePath)
	}
	}()
	log.Log.Infof("dump log file: %s", filePath)
	const bufferSize = 1024
	const maxBufferSize = 512 * bufferSize
	scanner := bufio.NewScanner(f)
	scanner.Buffer(make([]byte, bufferSize), maxBufferSize)
	for scanner.Scan() {
	log.Log.Info(scanner.Text())
	}
	if err := scanner.Err(); err != nil {
	log.Log.Reason(err).Errorf("failed to read file %s", filePath)
	}
	}

[EdDev]

Thank you for the examples @iholder101 , I think it gives a good representation of the problematic case, which IMO is not recommended, vs a good usage which solves a real need.

Lets start with the one I consider appropriate:
When a lambda is anonymous it usually comes to solve a context need, which in the example is a defer action and in other cases could be a scope one (e.g. using a lock with a release). There are also generic algorithm functions/methods that accept inputted functions as arguments that makes these a fit.

Another good usage is the power of closures, which allow one to store data in the function and use it to wrap things (e.g. convert foo(a,b) to boo(a) so it can be passed to a function parameter).

The reasoning that looks at reducing exposure is IMO a bad usage of the language feature.
It is actually a repeating patter I saw in other languages (e.g. python).
Languages that provide means to encapsulate data and implementation details, should use the most appropriate patterns to do so, and I personally consider this "hiding" pattern functions not to be it.

I can just mention that although Go does not have all the OO features, it has many of them in place. One of which is the ability to encapsulate data and methods in objects.
Therefore, if you are looking to hide things, use a struct with methods and state to do so.
These will usually not only solve the visibility but also allow proper tested code.

[0xFelix]

@iholder101 Agree, I did not want to express variable functions are a bad thing in general. But instead they should be used if they provide more value than a regular function. We have places in the code where you can simply move lambdas to regular functions without any changes necessary, and we should avoid such uses.

[iholder101]

Suggested change

	- Isolate code where possible
	- Isolate code where makes sense

[iholder101]

seems a bit too broad

[0xFelix]

Yes, I had these specific examples below in mind when writing this. I'll try to make this more specific.

[iholder101]

As I've claimed in this comment #11446 (comment), I think that comments are often dangerous and do more damage than good.

If your code is elegant in the sense that you have meaningful variable names, short and cohesive functions, and such, you wouldn't really need to use a lot of comments. Often comments are brought up to atone for low code quality. Obviously this is not always the case.

I would write something as follows:

Suggested change

	- Comment your code.
	- [Go's commenting conventions](http://blog.golang.org/godoc-documenting-go-code)
	- If reviewers ask questions about why the code is the way it is, that's a
	sign that comments might be helpful.
	- Write elegant, cohesive and easily readable code.
	- If reviewers ask questions about why the code is the way it is, that's a
	sign that your code is not clear enough.
	- Try improving your code on the expense of writing comments.
	- Add comments where it makes sense and where code-documentation is not enough.

[0xFelix]

Totally agree with that!

[dankenigsberg]

I love this initiative. However, I prefer to see a deeper discussion for each convention. How about introducing each convention in its own well-motivated commit? Alternatively, the reasons may be in-lined. Future readers of this page deserve to understand the motivation behind each convention, particularly if they ever intend to modify them.

[dankenigsberg]

The solution for a possible in-package conflict is to ensure that packages have a maintainable size (not too many files, not too long files) and that functions have clear useful names.

Defining a function inside another is a liability as

• it makes the caller function longer
• it forces a reader of the caller function to go through implementation details
• it forces a reader to wonder if the internal function is ever going to be replaced during runtime, adding to cognitive load.
• it risks capturing unintended variable from within the scope of the definition of the function
• it risks causing side effects on internal variables of the caller function

[fabiand]

10x for following up on this @0xFelix .

Dan is setting a high bar with requesting a motivation for each point. Along those lines I wonder if we can actually point to examples in our code base, to illustrate what we want to fix.

Having it in individual commits would be a cherry on top. But I do not mind that much on that point as long as the document is cpaturing the motivation.

[EdDev]

/cc

[codingben]

We have bash scripts and also golang scripts. Can you maybe add that separation like in Kubernetes example [1]?

[1] https://github.com/kubernetes/community/blob/master/contributors/guide/coding-conventions.md#code-conventions

[0xFelix]

Right, I forgot about that and will add something. But for now I wanted to focus more on the golang conventions.

[codingben]

From Kubernetes [1] doc: Ensure that build, release, test, and cluster-management scripts run on macOS. It can also refer to macOS that has arm64, currently even make fmt doesn't work on my arm64 laptop:

╭─laptop@laptop ~/GitHub/kubevirt ‹main› 
╰─$ make fmt
./hack/dockerized "hack/bazel-fmt.sh"
go version go1.21.5 linux/arm64

Error: statfs /Users/laptop/.docker/secrets: no such file or directory
make: *** [format] Error 125

[1] https://github.com/kubernetes/community/blob/master/contributors/guide/coding-conventions.md#code-conventions

[EdDev]

This is a great initiative, thank you very much!

I managed to go through the first 1/4 of the document so far.
I think it will become harder and harder to review this, as for almost every point we will have a discussion, sometime a long one.

One way to solve it, is to create a PR per point.
However, we better be practical here and try to optimize this.
I would suggest to call up a recurring meeting and discuss each point, asking members to come prepared with examples and/or presentations to promote their opinion.
We than can summarize the cons/pros formally and take a decision.
It will be a great opportunity to meet and learn from each other.

[EdDev]

The two linked ref are nice but I would prefer we focus on the major incidents we observe in our project and not to the whole Go ecosystem.

Said that, we can still ref these at the end of the document with recommendations for further reading.

[0xFelix]

Agree with that

[EdDev]

I did not see these occurring much in our project and while reading the gist I felt it may be better rephrased so more contributors will understand what to avoid.

I would add these to the end as well for further reading and phrase it directly in this document for the things you think are repeating all over the codebase and is not recommended.

[0xFelix]

👍

[EdDev]

Expect(functionThatDoesSomething(param1, param2, param3)).To(Succeed())

It looks like a big mess. A long one-liner that kind of hides the fact that we do an operation + validation at the same time.

From a human language point of view, Expect(RunSomething(a,b,c)).To(Succeed()) read better and focuses on the success of the call.
Furthermore, it supports functions that return a return value and an error, fitting cases when you do not care about the value (e.g. when creating a VMI object), you just care about its success.

[EdDev]

Makes sense to continue here, as that specific issue is one instance of many possible ones.

I would rephrase it a bit to express the problem.
E.g.

Use named return values when a good reason exists. Reason for it.

Then examples may be given for when it is a fit.
There are actually only a handful of reasons to use them and it is a good practice to see these uses as exceptions.

[EdDev]

This is actually enforced when appropriate by the current linter (if the code path is covered by it).

Documentation is needed when you have two returned values (or more) of the same type, to provide the documentation.
I think there is no other need for documentation when the function is simple enough and well named.
If the function is not well named and there are too many returned values, it is usually a hint of a problem and that should be treated instead.
But obviously there are cases when it makes total sense to return two strings and an error (as an example) and in those cases naming is good.

The main takeaway IMO from this should be: It is an exception to need it and one should try to avoid it because it is a smell.

[EdDev]

Thank you for the examples @iholder101 , I think it gives a good representation of the problematic case, which IMO is not recommended, vs a good usage which solves a real need.

Lets start with the one I consider appropriate:
When a lambda is anonymous it usually comes to solve a context need, which in the example is a defer action and in other cases could be a scope one (e.g. using a lock with a release). There are also generic algorithm functions/methods that accept inputted functions as arguments that makes these a fit.

Another good usage is the power of closures, which allow one to store data in the function and use it to wrap things (e.g. convert foo(a,b) to boo(a) so it can be passed to a function parameter).

The reasoning that looks at reducing exposure is IMO a bad usage of the language feature.
It is actually a repeating patter I saw in other languages (e.g. python).
Languages that provide means to encapsulate data and implementation details, should use the most appropriate patterns to do so, and I personally consider this "hiding" pattern functions not to be it.

I can just mention that although Go does not have all the OO features, it has many of them in place. One of which is the ability to encapsulate data and methods in objects.
Therefore, if you are looking to hide things, use a struct with methods and state to do so.
These will usually not only solve the visibility but also allow proper tested code.

[0xFelix]

From Kubernetes [1] doc: Ensure that build, release, test, and cluster-management scripts run on macOS. It can also refer to macOS that has arm64, currently even make fmt doesn't work on my arm64 laptop:

╭─laptop@laptop ~/GitHub/kubevirt ‹main› 
╰─$ make fmt
./hack/dockerized "hack/bazel-fmt.sh"
go version go1.21.5 linux/arm64

Error: statfs /Users/laptop/.docker/secrets: no such file or directory
make: *** [format] Error 125

[1] https://github.com/kubernetes/community/blob/master/contributors/guide/coding-conventions.md#code-conventions

While this is a valid point, I expect it will be very hard to actually implement it in this repository. I don't think KubeVirt will be able to run on macOS in the near future. The scripts for building/formatting/unit testing could be made to run on macOS though.

[0xFelix]

Thank you for all the comments and discussion on this. I will try to come up with a revised version of this implementing your suggestions. Mainly, I'll try to add rationales for all the stated conventions.

What do you think about keeping a rather brief list of conventions at the start of the document (think TL;DR) and giving detailed explanations in another section below?

When I think of myself starting work on a new project I don't want to read endless pages of conventions before getting started with the actual coding. Keeping the brief list at the start would provide a TL;DR to not lose the readers attention.

[fabiand]

Yes: Start small. Pick the top 5 things that new contributors should care about, and make them rich: Explain them well, point to existing examples in our code base, and the eventual fixes of them.

[0xFelix]

I pushed a new revision of the document and tried to take into account all the feedback I got. What do you think, is this going into the right direction?

[codingben]

I pushed a new revision of the document and tried to take into account all the feedback I got. What do you think, is this going into the right direction?

I'd say it looks much better! :)

[dankenigsberg]

thanks a lot for the new version, here's a partial review.

[dankenigsberg]

that's a problematic example, as an even better code would be

   return in == "B" || in == "C" 

[0xFelix]

Originally I was thinking of a more complex example not returning a bool. But yes, in case of bool return that is of course right.

[dankenigsberg]

that's a problematic example, as a constant must provide some kind of an abstraction over its value. We should never have

const Two = 2

We should name the constant such that if ever have to change 2 to 3, we do not need to change the name. Please provide a more concrete function and a clearer context for the constant.

[dankenigsberg]

Public constants are a hazard, particularly if they of a common type, because it is very easy to use them in incorrect context. I know that my argument here is weak, but to me, a public constant is a code smell.

For example, the fact that we have const EchoLastReturnValue in test/utils.go tells me that we should probably had something like console.CommandSuccessful(vmi, cmd) int.

[0xFelix]

Would you rather recommend carefully considering it? IMO there are still valid use cases for an exported constant.

[machadovilaca]

the terms when/if possible and when/where it makes sense are used very often, and in my opinion they don't add much to the convention description

p.e.:
Define variables in the function body if possible and where it makes sense.
could read
Prefer defining variables in the function body.

and

Avoid 'Skip' in tests if possible.
could read
Avoid 'Skip' in tests.

as it's written in the intro
Consider each convention a broad and opinionated statement. That means maintainers are allowed to make well-motivated exceptions, but it should not be the norm. so we can assume the if possibles and makes senses

[0xFelix]

Updated the doc with @EdDev suggestions. Any more comments on it?