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.

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

Rename instance()->instantiate() when it's a verb #49693

Merged
merged 1 commit into from
Jun 20, 2021

Conversation

AaronRecord
Copy link
Contributor

@AaronRecord AaronRecord commented Jun 17, 2021

@AaronRecord AaronRecord requested review from a team as code owners June 17, 2021 16:35
@AaronRecord AaronRecord marked this pull request as draft June 17, 2021 16:41
@YuriSizov
Copy link
Contributor

Shouldn't there be a change in the documentation as well?

@Xrayez
Copy link
Contributor

Xrayez commented Jun 17, 2021

I'm not particularly against the rename, but it's not only about simple refactor thing. The entire codebase should be reviewed for usage of "instance" word, including comments.

The existing "instance" word sounds fine to me (and is easier to type), but I'm not a native speaker. English allows to use nouns as verbs, so that's why it probably sounds natural to me:

https://en.wikipedia.org/wiki/Buffalo_buffalo_Buffalo_buffalo_buffalo_buffalo_Buffalo_buffalo

It also depends on how you interpret method names. instance() can be used as a noun for an action that returns the already instantiated object, like in next_instance() (does not exist in Godot, that's just to give an example). instance() can also be interpreted as a shorthand for create_instance(), where create implicitly acts as a verb.

I guess it's a problem of being explicit and being too verbose. That's why I prefer that we don't rename existing methods. But if we do, we should recognize the right context and rename existing methods accordingly (with the possiblity to remain existing instance() name as-is, where appropriate).

@YuriSizov
Copy link
Contributor

YuriSizov commented Jun 17, 2021

The existing "instance" word sounds fine to me (and is easier to type), but I'm not a native speaker. English allows to use nouns as verbs, so that's why it probably sounds natural to me

Yes, it does sound valid, but probably because "instance" can actually be used as a verb, but it has an unrelated meaning.

@AaronRecord AaronRecord force-pushed the instance-is-not-a-verb branch 2 times, most recently from 8799e77 to 46ce7fa Compare June 17, 2021 21:09
@AaronRecord AaronRecord changed the title Rename "instance"->"instantiate" when it's a verb Rename instance()->instantiate() when it's a verb Jun 17, 2021
@AaronRecord AaronRecord force-pushed the instance-is-not-a-verb branch 2 times, most recently from 32ee0ca to 4b7a5b7 Compare June 17, 2021 21:24
@AaronRecord
Copy link
Contributor Author

AaronRecord commented Jun 17, 2021

I think instantiate() makes more sense than instance() for meaning "create an instance of ...". Without context, instance() could mean get_instance() or is_instance().

@AaronRecord AaronRecord force-pushed the instance-is-not-a-verb branch 4 times, most recently from 644ee59 to 76309c1 Compare June 17, 2021 22:16
@fire
Copy link
Member

fire commented Jun 17, 2021

I can see myself typoing a bit with this compared to instance.

Would mistakenly rearrange the vowels.

@AaronRecord
Copy link
Contributor Author

AaronRecord commented Jun 18, 2021

Should I open a proposal for this? The comment has +24 -0 likes/dislikes, so I'm not sure if it's necessary.

@AaronRecord AaronRecord force-pushed the instance-is-not-a-verb branch 2 times, most recently from 3230a13 to efa8e53 Compare June 18, 2021 00:17
@AaronRecord AaronRecord requested review from a team as code owners June 18, 2021 15:49
@AaronRecord AaronRecord force-pushed the instance-is-not-a-verb branch 7 times, most recently from 842755a to 683ad7c Compare June 19, 2021 22:14
@AaronRecord
Copy link
Contributor Author

AaronRecord commented Jun 19, 2021

(rebased following #44806)

@akien-mga
Copy link
Member

Needs another rebase as I merged a bunch of PR (+ a couple might have added new calls to instance() to change).

Then I'll merge this PR first :)

@AaronRecord AaronRecord force-pushed the instance-is-not-a-verb branch 6 times, most recently from 6d98dcf to 6364f23 Compare June 20, 2021 00:40
@AaronRecord AaronRecord marked this pull request as draft June 20, 2021 00:42
@AaronRecord AaronRecord marked this pull request as ready for review June 20, 2021 02:49
@AaronRecord
Copy link
Contributor Author

Okay I think everything should be good to go.

@akien-mga akien-mga merged commit 4fcc589 into godotengine:master Jun 20, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants