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

Unify the DoWith function behavior #4266

Open
madmaxoft opened this Issue Jul 26, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@madmaxoft
Copy link
Member

madmaxoft commented Jul 26, 2018

All the DoWith functions should adhere to the same behavior - they should return false if the object is not found, and they should return the value from the callback when the object was found.

Ref.: #4259 (comment)

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Jul 26, 2018

Functions (checked if already adhering):

  • cPluginManager::DoWithPlugin
  • cBlockArea::DoWithBlockEntityAt
  • cBlockArea::DoWithBlockEntityRelAt
  • cChunkMap::DoWithChunk
  • cChunkMap::DoWithChunkAt
  • cMapManager::DoWithMap - returns true
  • cRoot::DoWithPlayerByUUID - returns true, depends on cWorld::DoWithPlayer
  • cRoot::DoWithPlayer - returne true, depends on cWorld::DoWithPlayer
  • cWorld::DoWithBlockEntityAt - inverted
  • cWorld::DoWithBeaconAt - inverted
  • cWorld::DoWithBedAt - inverted
  • cWorld::DoWithBrewingstandAt - inverted
  • cWorld::DoWithChestAt - inverted
  • cWorld::DoWithChunk
  • cWorld::DoWithChunkAt
  • cWorld::DoWithCommandBlockAt - inverted
  • cWorld::DoWithDispenserAt - inverted
  • cWorld::DoWithDropperAt - inverted
  • cWorld::DoWithDropSpenserAt - inverted
  • cWorld::DoWithEntityByID
  • cWorld::DoWithFlowerPotAt - inverted
  • cWorld::DoWithFurnaceAt - inverted
  • cWorld::DoWithMobHeadAt - inverted
  • cWorld::DoWithNoteBlockAt - inverted
  • cWorld::DoWithPlayer - returns true
  • cWorld::DoWithPlayerByUUID - returns true
@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Jul 26, 2018

What a mess

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Jul 26, 2018

The funny thing is, functions that do invert the callback return value do so while breaking the docs, the cWorld::DoWith function have this in their API docs:

The function returns false if there is no chest, or if there is, it returns the bool value that the callback has returned.

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Jul 26, 2018

And the doxy-comments agree with the code but not the Lua API docs:

/** Calls the callback for the dispenser at the specified coords; returns false if there's no dispenser at those coords or callback returns true, returns true if found */

@Seadragon91

This comment has been minimized.

Copy link
Contributor

Seadragon91 commented Jul 29, 2018

Maybe the pull request could also include to check if the lua code was run successfully:

L.Call(FnRef, &a_Item, cLuaState::Return, ret);

As there is no check if the lua code failed, this can result in unnecessary calls. This should be done in all places.

Example:

cRoot:Get():GetDefaultWorld():ForEachEntity(
	function(a_Entity)
		a_Entity:Infinity()
	end)

This would run the callback for all entities with an call to an non-existent function.

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 2, 2018

Another possible way to tackle this would be to separate the two values and report both:

std::pair<bool, bool> DoWithXYZ(...);

The first value tells whether the object was found, the second value is what the callback returned (or false if object not found?)
The Lua API could reflect this by having two return values.

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 2, 2018

Or maybe an enum: True, False, FileNotFound.

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