Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unification of receiver names #1186

Merged
merged 1 commit into from
Jan 13, 2020
Merged

Unification of receiver names #1186

merged 1 commit into from
Jan 13, 2020

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Nov 11, 2019 •
edited
Loading

Linter is reporting different receiver names (i.e., methods of same struct with different receiver names) as if they were generic. Additionally most of the files use generic receivers such as a. Tiis PR changes all receiver names for implementations fo the File interface to f as a mnemonic of file.

Linter message:

Receiver has generic name inspection

Lister description:

Reports receiver names like me, this, self or different from other receiver names for this type.
See Code review comments: receiver names for details.

Closes: #1182

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 11, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @Adirio. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 11, 2019
@Adirio
Copy link
Contributor Author

Adirio commented Nov 11, 2019

Note to reviewers: do not get scared with the number of files modified. They are just pointer name replacements to use f for all of them instead of a, c, g, ...

@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 12, 2019 •
edited
Loading

Hi @Adirio,

It shows good, however, see that is not passing in the CI. Shows that it is missing a rebase with the master to pass in the CI. Also, I'd recommend you add here in the first comment the error/warning from the linter used to get it which shows that is your motivation of Umbrella issue: #1176

@camilamacedo86
Copy link
Member

camilamacedo86 commented Nov 12, 2019 •
edited
Loading

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 12, 2019
@camilamacedo86
Copy link
Member

/assign @mengqiy

@camilamacedo86
Copy link
Member

/assign @camilamacedo86

@Adirio
Copy link
Contributor Author

Adirio commented Nov 12, 2019

/retest

@Adirio
Copy link
Contributor Author

Adirio commented Nov 12, 2019

Can someone re-trigger Travis? It gave an error trying to find a file that hasn't been touched and is in the expected path.

@camilamacedo86
Copy link
Member

HI @Adirio,

Can someone re-trigger Travis? It gave an error trying to find a file that hasn't been touched and is in the expected path.

If you close and re-open the PR it will be re-trigged.

@Adirio Adirio closed this Nov 12, 2019
@Adirio Adirio reopened this Nov 12, 2019
@Adirio Adirio mentioned this pull request Nov 13, 2019
Closed
@Adirio Adirio force-pushed the unify-receiver-names branch 2 times, most recently from 49593d6 to c7ff4fb Compare November 18, 2019 10:48
@Adirio
Copy link
Contributor Author

Adirio commented Nov 22, 2019

Hi @camilamacedo86,

It shows good, however, see that is not passing in the CI. Shows that it is missing a rebase with the master to pass in the CI. Also, I'd recommend you add here in the first comment the error/warning from the linter used to get it which shows that is your motivation of Umbrella issue: #1176

The rebase was done and the CI is passing correctly. Description was also updated. Do you have any other review or is this PR ready to merge?

camilamacedo86
camilamacedo86 approved these changes Nov 22, 2019
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with. Should not change the current behaviour and is passing in the CI.
Besides it to be in many files the change is small and straight forward. @mengqiy wdyt?

/lgtm /approved

@Adirio
Copy link
Contributor Author

Adirio commented Nov 26, 2019

Rebased (no changes)

@Adirio
Copy link
Contributor Author

Adirio commented Jan 8, 2020 •
edited
Loading

In case f for file sounds too generic for the 73 File

OOOOOOHHHHH. That's the mnemonic. Sorry, that was lost during code review ;-). f is fine for file. That makes a whole lot more sense now :-)

I thought I mentioned it somewhere. You know, when you change it in 73 files, it is easy to take gfor granted that the mnemonic is obvious. Added it to the PR description for future reference.

(just needs a rebase)

@DirectXMan12 rebase done, could you add the LGTM label again?

@Adirio
Copy link
Contributor Author

Adirio commented Jan 8, 2020

Travis error seems to be related to #1016 being recently merged.

@camilamacedo86
Copy link
Member

HI @Adirio,

The error shows occur because the files are not updated. I'd recommend you do the following steps. I think it will solve.

  1. Rebase with the master
  2. Run make generate (it will do all tricks to run the golden script properly)

@Adirio
Copy link
Contributor Author

Adirio commented Jan 8, 2020

This already was rebased, and it doesn't change anything in the testdata folder. The problem seems to be introduced by an old PR that was merged yesterday.

@Adirio
Copy link
Contributor Author

Adirio commented Jan 8, 2020 •
edited
Loading

A fix to the Travis error has been provided in #1288, it shouldn't need a rebase afterwards.
A separate PR has been used for the fix to separate the topics.

@Adirio Adirio closed this Jan 10, 2020
@Adirio Adirio reopened this Jan 10, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Jan 10, 2020

Rebased to apply the fix to Travis. This should only need a lgtm label.

@camilamacedo86 @DirectXMan12

@mengqiy
Copy link
Member

mengqiy commented Jan 10, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Jan 11, 2020

I will resolve the conflicts tomorrow

/hold cancel

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 11, 2020
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 11, 2020
@Adirio
Copy link
Contributor Author

Adirio commented Jan 11, 2020

@mengqiy Rebased and ready, only requires LGTM label

This was referenced Jan 11, 2020
Merged
Closed
@Adirio
Copy link
Contributor Author

Adirio commented Jan 11, 2020

Travis error seems unrelated, please take a look at #1303.

@Adirio
Copy link
Contributor Author

Adirio commented Jan 13, 2020

This PR has been approved and every test passes now. Can someone add again the lgtm label as it was removed in a rebase?

@Adirio
Copy link
Contributor Author

Adirio commented Jan 13, 2020

@camilamacedo86

@camilamacedo86
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2020
@k8s-ci-robot k8s-ci-robot merged commit c093dd1 into kubernetes-sigs:master Jan 13, 2020
@Adirio Adirio deleted the unify-receiver-names branch January 13, 2020 14:00
@Adirio Adirio mentioned this pull request Jan 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@Liujingfang1 Liujingfang1 Awaiting requested review from Liujingfang1

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@DirectXMan12 DirectXMan12

@mengqiy mengqiy

@camilamacedo86 camilamacedo86

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unification of receiver names
5 participants
@Adirio @k8s-ci-robot @camilamacedo86 @DirectXMan12 @mengqiy