mobile icon indicating copy to clipboard operation
mobile copied to clipboard

Fix setter generation for ObjC and update golden files

Open triztian opened this issue 5 years ago • 16 comments

triztian avatar Jul 20 '19 07:07 triztian

This PR (HEAD: 930e9a0e0ef46b2f44182886a9effcc6642c79ea) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/mobile/+/186978 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

gopherbot avatar Jul 20 '19 07:07 gopherbot

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps: Within the next week or so, a maintainer will review your change and provide feedback. See https://golang.org/doc/contribute.html#review for more info and tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be surprising to people new to the project. The careful, iterative review process is our way of helping mentor contributors and ensuring that their contributions have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which little code gets reviewed or merged. If a reviewer responds with a comment like R=go1.11, it means that this CL will be reviewed as part of the next development cycle. See https://golang.org/s/release for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Jul 20 '19 07:07 gopherbot

This PR (HEAD: bdb69961fe81e3ed429cb165dd66551b95822b7f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/mobile/+/186978 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

gopherbot avatar Jul 20 '19 19:07 gopherbot

Message from Hyang-Ah Hana Kim:

Patch Set 2:

(3 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Jul 29 '19 20:07 gopherbot

Message from Tristian Azuara:

Patch Set 2:

Patch Set 2:

(3 comments)

Hi Yang-Ah,

Thank you for reviewing my code!.

I had done a forced push that tries to follow the mentioned guidelines, however it does not seem to have updated the tip the branch that was imported to gerrit.

https://github.com/triztian/mobile/commit/bdb69961fe81e3ed429cb165dd66551b95822b7f

The linked issue is the following one:

  • https://github.com/golang/go/issues/32008

Should I do a "normal" commit instead of a force push with amended (--amend) changes?, or is there a way to update the CL with the forced pushed updates?

Thank you for your time!


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Jul 29 '19 20:07 gopherbot

Message from Hyang-Ah Hana Kim:

Patch Set 2:

Patch Set 2:

Patch Set 2:

(3 comments)

Hi Yang-Ah,

Thank you for reviewing my code!.

I had done a forced push that tries to follow the mentioned guidelines, however it does not seem to have updated the tip the branch that was imported to gerrit.

https://github.com/triztian/mobile/commit/bdb69961fe81e3ed429cb165dd66551b95822b7f

The linked issue is the following one:

  • https://github.com/golang/go/issues/32008

Should I do a "normal" commit instead of a force push with amended (--amend) changes?, or is there a way to update the CL with the forced pushed updates?

Thank you for your time!

I am not familiar with cl push through github, but according to https://golang.org/doc/contribute.html#sending_a_change_github either is fine, and just pushing to the same branch will do the right job. If not, let me know.


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Jul 30 '19 14:07 gopherbot

Message from Hyang-Ah Hana Kim:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Jul 30 '19 14:07 gopherbot

This PR (HEAD: 50acf06964954ba75f55cb1f3ca49ad6da42b79b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/mobile/+/186978 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

gopherbot avatar Aug 02 '19 04:08 gopherbot

Message from Tristian Azuara:

Patch Set 4: Commit message was updated.


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Aug 02 '19 04:08 gopherbot

Message from Tristian Azuara:

Patch Set 3:

Patch Set 2:

(1 comment)

Hi Hyang-Ah

Sorry for the late feedback, busy week.

For this issue not only do we have to title case the first rune but we also have to lower case the 2nd rune (if any), ToTitle will title case the first rune only but not lower case the second one.

I've done an update that includes the greek language test case. I've also updated the commit message using the "edit" button in gerrit, I never figured out how to make gerrit import the commit message from a forced push.


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Aug 02 '19 04:08 gopherbot

This PR (HEAD: 5b8e53aeedb9ba174576dd5e59531e06e0174452) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/mobile/+/186978 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

gopherbot avatar Aug 02 '19 04:08 gopherbot

Message from Tristian Azuara:

Patch Set 7: Commit message was updated.


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Aug 02 '19 05:08 gopherbot

Message from Hyang-Ah Hana Kim:

Patch Set 8:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Aug 02 '19 12:08 gopherbot

Message from Hyang-Ah Hana Kim:

Patch Set 8:

Patch Set 3:

Patch Set 2:

(1 comment)

Hi Hyang-Ah

Sorry for the late feedback, busy week.

For this issue not only do we have to title case the first rune but we also have to lower case the 2nd rune (if any), ToTitle will title case the first rune only but not lower case the second one.

I've done an update that includes the greek language test case. I've also updated the commit message using the "edit" button in gerrit, I never figured out how to make gerrit import the commit message from a forced push.

https://github.com/golang/go/wiki/GerritBot#how-does-gerritbot-determine-the-final-commit-message Can you update the PR title&description?


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Aug 02 '19 15:08 gopherbot

Message from Tristian Azuara:

Patch Set 8:

(1 comment)

Patch Set 8:

(1 comment)

You are correct in the expected name for the setter, however "set"+strings.Title(lowerFirst(f.Name())) would yield "setOSName" instead of "setOsName" when the name of the declared Go field is OSName the second rune has to be explicitly lower cased and the 1st title cased. There would not be an issue if the Go field is named osName.


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Aug 02 '19 21:08 gopherbot

Message from Go Bot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps: Within the next week or so, a maintainer will review your change and provide feedback. See https://golang.org/doc/contribute.html#review for more info and tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be surprising to people new to the project. The careful, iterative review process is our way of helping mentor contributors and ensuring that their contributions have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which little code gets reviewed or merged. If a reviewer responds with a comment like R=go1.11, it means that this CL will be reviewed as part of the next development cycle. See https://golang.org/s/release for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/186978. After addressing review feedback, remember to publish your drafts!

gopherbot avatar Oct 15 '20 03:10 gopherbot