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

Add optional default value to get_meta() #58608

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Feb 27, 2022

@KoBeWi KoBeWi added enhancement topic:core cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 27, 2022
@KoBeWi KoBeWi added this to the 4.0 milestone Feb 27, 2022
@KoBeWi KoBeWi requested review from a team as code owners February 27, 2022 21:07
@Calinou
Copy link
Member

Calinou commented Feb 27, 2022

I'd prefer to add an optional third argument to get_meta(), which would be more similar to Dictionary.get().

Having a separate method is warranted for having get_meta_or_null() in the future (once we make get_meta() return an error when the value doesn't exist and no default is specified). But for returning a default value, I wouldn't use a separate method entirely.

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 27, 2022

get_meta_or_null()

I don't think having such method makes sense. We have get_node_or_null() because you can literally only return either node or null. In case of Dictionary or meta, the default value can be any Variant. And returning null in case of missing value means that you have to add more code if you want a different default.

Having a separate method is warranted for having get_meta_or_null() in the future (once we make get_meta() return an error when the value doesn't exist and no default is specified).

This basically means that we'd add get_meta_or_null() just to have a version of get_meta() with null default value that doesn't throw error. Instead of having two methods with default for such a weird reason, it's better to have one.

@akien-mga
Copy link
Member

We discussed in PR review meeting that this makes sense, but could actually be done by adding the default value to get_meta itself, defaulting to null. If it's null (no custom default set by the user), it would throw an error when the meta isn't found. Otherwise, it returns the non-null default.

As mentioned in the PR review, that doesn't cover the use case that get_meta_or_default(meta, null) would, but we thought that this specific use case doesn't necessarily need to be supported (and can be handled with has_meta).

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 22, 2022

I checked my code for use of get("sth", null) and found this:

var icon = icon_cache.get(tab_scenes[i], null)
if icon:
	slot.set_texture(icon)
else:
	generate_icon(instance as Node2D, slot)

The get_meta() equivalent would be:

if node.has_meta("meta"):
    value = node.get_meta("meta")
else:
    value = something_else

In such cases I need to access the "meta" twice. But I guess it's rare enough to live with that. I'll make the discussed changes.

@EricEzaM
Copy link
Contributor

EricEzaM commented Mar 22, 2022

As mentioned in the PR review, that doesn't cover the use case that get_meta_or_default(meta, null) would, but we thought that this specific use case doesn't necessarily need to be supported (and can be handled with has_meta).

Tbh I think the _or_default method is useful and covers a lot of cases. Searching for has_meta( in the code base returns a number of results where this new method could be used to simplify the code. I think adding a param to get_meta (as described in quoted text above) but not having it work (without error) 100% of the time is a bit of a half measure.

If it were my choice I would either have the _or_default version, or veto this overall.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 22, 2022

adding the default value to get_meta itself

Done.

@akien-mga
Copy link
Member

Does this need a rebase after #59452 or is it fine as is?

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Approved based on what we discussed last time.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 29, 2022

Tested, still works.

@akien-mga akien-mga merged commit 482cdea into godotengine:master Mar 29, 2022
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the metadefault branch March 29, 2022 14:16
@KoBeWi KoBeWi mentioned this pull request Apr 1, 2022
@akien-mga
Copy link
Member

Cherry-picked for 3.5.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Apr 13, 2022
@akien-mga akien-mga changed the title Add get_meta_or_default() Add optional default value to get_meta() Aug 4, 2022
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.

Allow get_meta() to return a default value (similar to Dictionary.get())
4 participants