dev: rename function parameter i to issue

[alexandear]

The PR renames the identifier i to enhance readability and maintainability.

It mostly renames i *result.Issue to issue *result.Issue. Other changes include newI to newIssue, copyI to copyIssue.

[ldez]

I will decline this PR, i is the right name for the receiver.

[bombsimon]

I think the changes that was receiving arguments and variables makes sense. Single letter variables for receiving type is best practices but not as arbitrary variable in the code or as receiving argument afaik?

[ldez]

Not sure to understand 🤔

One letter for a receiver is best practice.

The problem is one letter as a variable. (i -> index)

I don't understand your message.

[bombsimon]

I guess it's subjective but for me:

// Receiving type, more common over `issue`
func (i Issue) Fn() {}

// Not the receiving type, more common than `i`
func Fn(issue Issue) {}

Example from stdlib (not the best source always but hey), they use f and buf, not f and b in func (f *File) Read(buf []byte).

I guess it depends on the type as well, you don't have much context over the type []byte but you do over Issue. I still just think it's good practice with somewhat descriptive variable names in code and as arguments but that's probably a personal preference.

Here's an article mentioning the same topic: https://medium.com/@TonyBologni/go-bits-variable-names-fa8240284edc. I notice that it actually refers to the wiki (that's now moved) that somewhat suggest otherwise so maybe I'm wrong here.

[ldez]

🤔 do you mean that the variable i (from the for) should be renamed to index?

func (i *Issues) Validate() error {
	for i, rule := range i.ExcludeRules {
		if err := rule.Validate(); err != nil {
			return fmt.Errorf("error in exclude rule #%d: %w", i, err)
		}

[ldez]

https://go.dev/wiki/CodeReviewComments#receiver-names

https://go.dev/wiki/CodeReviewComments#variable-names

There is a "fight" between the 2 rules from my POV about i as receiver names and index.

[alexandear]

I agree that i is acceptable as a receiver name. I can revert all the changes made to the receiver and only rename parameters: i to issue, newI to newIssue, and copyI to copyIssue. Would that be acceptable?

[ldez]

I understand the root of my confusion: PR contains several changes.

• argument names
• receiver names
• variable names

I was focused on the receiver names is

[bombsimon]

I guess I'm just not consistent here so maybe dismiss my argument here. In general I wouldn't rename i in a for loop because it's such a well known pattern however in this case I probably would to avoid shadowing. Partly this is a reason for me commenting because I think if you can avoid i you might as well do that to avoid confusion.

Maybe this is not a rule that you can always apply, I wouldn't think func addTwo(n int) { return n + 2 } would be weird because it's such a small and simple function. The reason I commented was because I think a lot of the changes is in a place where i is used in a bigger scope and context and having a better name is helping readability. F.ex. newI := *i vs. newIssue := *issue is an improvement to me.

[ldez]

Sorry it's my fault for misreading the PR.
I was misled by the description.

[ldez]

@alexandear can you rebase your PR to fix the conflict?

[ldez]

I'm still not convinced by the receiver name is (it's weird because is is a word related to boolean).

I think we can keep the other 2 letters receiver but is causes me trouble 😄

[alexandear]

Rebased and left only changes where i renamed to issue.

[ldez]

LGTM

Sorry again for misreading the PR.