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 nodeupdate and nodeupdate_single and make them part of the official API #4767

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@est31
Copy link
Contributor

est31 commented Nov 12, 2016

Now, the renamed forms of nodeupdate and nodeupdate_single are part of the official API.

As nodeupdate has been used by Minetest Game and in mods despite of not
being part of the official API, we ease the transition by still supporting
it for the 0.4.15 release. After the release, the two functions can be removed.

The removal will not violate the stability promise, as that promise only
includes the official and documented API.

Also, make some formerly global functions local. They most likely haven't
been used by mods, therefore they won't get stubs with deprecation warnings,
hard erroring directly.

See issue #4733 and PR #4753.

@est31 est31 added the Maintenance label Nov 12, 2016

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

Wuzzy2 commented Nov 12, 2016

👍 for only deprecating nodeupdate and nodeupdate_single while still adding new function names.
I would, however, write the deprecation explicitly into the changelog because this change is easy to miss.

builtin/game/falling.lua Outdated
check_single_for_falling(p)
end

function core.check_for_falling(p)

This comment has been minimized.

Copy link
@paramat

paramat Nov 13, 2016

Member

Is doing this a Lua optimisation? If so should we be doing this everywhere from now on? and maybe edit builtin to do this throughout in another PR?

This comment has been minimized.

Copy link
@est31

est31 Nov 13, 2016

Author Contributor

@paramat I'm not sure anymore why I did this, but I agree it certainly was bad, its not really a speed enhancement.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Nov 13, 2016

Line comment, but looks good.

@est31 est31 force-pushed the est31:falling branch Nov 13, 2016

builtin/game/falling.lua Outdated
@@ -98,7 +98,7 @@ core.register_entity(":__builtin:falling_node", {
core.add_node(np, self.node)
end
self.object:remove()
nodeupdate(np)
check_for_falling(np)

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Nov 13, 2016

Member

Shouldn't it be core.check_for_falling?

This comment has been minimized.

Copy link
@est31

est31 Nov 13, 2016

Author Contributor

Thanks for pointing out

@est31 est31 force-pushed the est31:falling branch Nov 13, 2016

doc/lua_api.txt Outdated
* `core.check_for_falling(pos)`
* causes an unsupported `group:falling_node` node to fall and causes an
unattached `group:attached_node` node to fall.
* Spread these updates to neighbours and can cause a cascade

This comment has been minimized.

Copy link
@paramat

paramat Nov 14, 2016

Member

Minor point, capital 'S' is inconsistent.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Nov 14, 2016

Minor line comment, however 👍

Rename nodeupdate and nodeupdate_single and make them part of the off…
…icial API

Now, the renamed forms of nodeupdate and nodeupdate_single are part of the official API.

As nodeupdate has been used by Minetest Game and in mods despite of not
being part of the official API, we ease the transition by still supporting
it for the 0.4.15 release. After the release, the two functions can be removed.

The removal will not violate the stability promise, as that promise only
includes the official and documented API.

Also, make some formerly global functions local. They most likely haven't
been used by mods, therefore they won't get stubs with deprecation warnings,
hard erroring directly.

@est31 est31 force-pushed the est31:falling branch to 6b20118 Nov 14, 2016

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented Nov 14, 2016

Adding own approval

@est31 est31 added >= Two approvals and removed One approval labels Nov 14, 2016

@Zeno-

This comment has been minimized.

Copy link
Contributor

Zeno- commented Nov 14, 2016

👍

1 similar comment
@paramat

This comment has been minimized.

Copy link
Member

paramat commented Nov 14, 2016

👍

@est31

This comment has been minimized.

Copy link
Contributor Author

est31 commented Nov 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.