search

lima-vm / lima

Linux virtual machines, with a focus on running containers
https://lima-vm.io/
Apache License 2.0
15.07k stars 591 forks source link

pkg: improve field and variable names #2623

Closed alexandear closed 1 week ago

alexandear commented 1 week ago

This PR renames y to instConfig because y is too short for its usage scope, which broke the CodeReviewComments#variable-names statement:

The basic rule: the further from its declaration that a name is used, the more descriptive the name must be

jandubois commented 1 week ago

I don't think yaml is such a good name either. It describes a data format, not what the data is supposed to represent. And the variable has nothing to do with YAML beyond that the struct has been loaded from a YAML file/string.

So if we rename y to something longer, how about instConfig, which is at least descriptive? And it goes well with instName and instDir, which we already use.

afbjorklund commented 1 week ago

Renaming the field is OK, but I don't like not being able to use short variable names (like i, j and k).

Variable names in Go should be short rather than long. This is especially true for local variables with limited scope. Prefer c to lineCount. Prefer i to sliceIndex.

alexandear commented 1 week ago

So if we rename y to something longer, how about instConfig, which is at least descriptive? And it goes well with instName and instDir, which we already use.

Totally agree with you. Renamed to the instConfig.

alexandear commented 1 week ago

Renaming the field is OK, but I don't like not being able to use short variable names (like i, j and k).

It's okay to use i, j, k in for loops where the variable's scope is short, 2-4 lines. Also, one-letter vars are acceptable for receiver names.

It's not okay to use y as the variable name in our case, where the scope is 83 lines.

The Google Go Style Guide contains explanations on variable naming.

jandubois commented 1 week ago

Renaming the field is OK, but I don't like not being able to use short variable names (like i, j and k).

I think it is something to decide on a case-by-case basis. Sometimes the short names work fine, either when their declaration and use case all fits on a single screen, or when the variable is used all the time and not just sparingly.

I could go with cfg for a local variable, but I think using instConfig consistently is better in this case.

Note also how FillDefault continues to use y (and o and d) and has not been modified, because there it is used in every other line, and a longer name would just drown out the field names.

jandubois commented 1 week ago

@alexandear Please resolve conflicts.

@afbjorklund Do you still object to the renaming of local variables in this PR, or was that more a general sentiment.

I'll file a separate issue (#2636) to investigate why the BaseDriver type has both a store.Instance and a limayaml.LimaYAML field; that can be sorted out once this PR has been merged.

afbjorklund commented 17 hours ago

@jandubois it was just a general sentiment, but it also applied to parts of this and now I have a lot of rebasing to do*

* that's not an issue with this particular change, more of a downside of having multiple long-lived "branches" outside