[0.15] [Rseding91] LuaEntity:neighbours on underground belt returns {} not nil

[sparr]

"When called on an underground transport belt, this is the other end of the underground belt connection, or nil if none."

This changed at some point and now seems to return {} instead of nil for an unconnected underground input. Probably should just be fixed in the docs.

[Rseding91]

Thanks for noticing. It's now fixed for the next version of 0.15.

[sparr]

Can you say if you're updating the docs or switching the code back from {} to nil?

I already updated one mod to support either option, but I'd prefer to update others "correctly".

[Rseding91]

Making it return what the docs say so "nil" or the entity for underground belts.

[dehook]

Regardless of what the docs say, returning an empty collection generally allows for more concise client code than returning nil. This change has also broken at least two mods that used that more concise style.

[sparr]

How long was this bug in effect? If it happened in maybe 0.15.9 then reverting it makes sense. If it happened in 0.14.0 and no one noticed until now but multiple mods have been written in the last year that use the {} behavior, then maybe {} is the better option.

[Rseding91]

The electric pole result being a dictionary already means the return result must be inspected before it can be meaningfully used.

I don't really mind either way but it doesn't seem like one format or the other gives any advantage.

[Klonan]

Regardless of what the docs say, returning an empty collection generally allows for more concise client code than returning nil. This change has also broken at least two mods that used that more concise style.

I find that returning nil is generally far easier to work with,

Code: Select all

if entity.neighbors then ...

is much simpler than

Code: Select all

if entity.neighbors[1] then... 

The electric pole result being a dictionary already means the return result must be inspected before it can be meaningfully used.

I don't really mind either way but it doesn't seem like one format or the other gives any advantage.

I would keep it as nil, as it is more consistent with the way most of the other API calls work

[dehook]

I find that returning nil is generally far easier to work with,

The savings comes in something like this from an effected mod:

Code: Select all

  for _, neighbour in pairs(entity.neighbours) do...

If entity.neighbours is an empty collection, then the for loop just doesn't do anything with no check for nil or empty required.

[gheift]

When it returns nil, just expand the code like this:

Code: Select all

for _, neighbour in pairs(entity.neighbours or {}) do...

[sparr]

Apparently this bug existed for a while. Changing it back broke the following mods for me:

Orphan Finder
Fast Replace Underground Belts

[sparr]

Code: Select all

local function has_neighbours(entity)
  return not (entity == nil or entity.neighbours == nil or (#entity.neighbours == 0 and not entity.valid))
end

[Optera]

Directly returning the other ug belt entity seems a bit counter intuitive to the the description.

neighbours :: dictionary string → array of LuaEntity or array of LuaEntity or LuaEntity [Read-only]

I would expect a dictionary containing the entity instead.

It should be:
neighbours :: → array of LuaEntity or array of LuaEntity or LuaEntity [Read-only]

[Nexela]

I agree this "change" leads to some ugly hacks The way I thought of neighbours before this change was "Hey I want to visit the next(neighbours)
The plural s on neighbours makes my mind thing it is going to return a table with 0 or more items

before when using it on both pipes and undergrounds

if neighbours and #neighbours < 1 then ------

now I have to do some wonky stuff like compare types

if not neighbours or neighbours and not neighbours.type and #neighbours < 1 then -----

@optera
dictionary string → array of LuaEntity is for electrical poles {red = {}, copper = {}, green = {}}

[Narc]

I agree this "change" leads to some ugly hacks [...] before when using it on both pipes and undergrounds

if neighbours and #neighbours < 1 then

Shouldn't that still work? I mean, 'if not neighbours' will be true if `if neighbours == nil` is true.

Code: Select all

local function has_neighbours(entity)
  return not (entity == nil or entity.neighbours == nil or (#entity.neighbours == 0 and not entity.valid))
end

I always like my sanity checks separate from the actual logic, so

Code: Select all

local function has_neighbours(entity)
    if not entity or not entity.valid then return false end
    if not entity.neighbours then return false end

    return entity.neighbours < 1
end

[Nexela]

Code: Select all

    local function has_neighbours(entity)
        if not entity or not entity.valid then return false end
        if not entity.neighbours then return false end

        return entity.neighbours < 1
    end

assuming your return is just missing the #
on an underground belt that will return 0 if the belt has neighbours since the neighbour is an entity

[Narc]

assuming your return is just missing the #

Yup, sorry, untested code. It's also got the logic backward, and would fit "has_no_neighbours(entity)"

on an underground belt that will return 0 if the belt has neighbours since the neighbour is an entity [and entities have 0 array members, but still answer to #table]

Oh, yuck, I see what you mean. Might be a dictionary, might be an array, might be a single entity depending on the type. In fact, let's not forget: might raise an error if used on an entity that doesn't support it.

Code: Select all

local function has_neighbours(entity)
    if not entity or not entity.valid then return false end
    if not entity.neighbours then return false end

    if entity.type == "underground-belt" then
        return entity.neighbours and entity.neighbours.valid
    elseif entity.type == "electric-pole" then
        return #entity.neighbours.copper > 0 or #entity.neighbours.red > 0 or #entity.neighbours.green > 0
    else
        return #entity.neighbours > 0
    end
end

Oh, and this will still raise an error if used on an entity that doesn't support .neighbours at all. I just don't even at this point.

[Nexela]

I miss next(neighbours) already

[Optera]

sanity check for ug belts actually is really simplistic now:

Code: Select all

if entity.type == "underground-belt" and entity.neighbours then 

My problem is like Nexela said, neighbours is plural indicating an array.
Also it returns 3 different things for the 3 different entity types it can be used on.

[sparr]

adding my opinion to the side that "neighbourS" ever returning a single entity, rather than a 1-entity array, makes for ugly code.