lunarmodules / luasocket

Network support for the Lua language
http://lunarmodules.github.io/luasocket/
MIT License
1.85k stars 629 forks source link

fix(inet): Return port as number in getsockname #392

Closed georgeto closed 12 months ago

georgeto commented 2 years ago

The documentation of getsockname() states that the port is returned as a number, which is also what the similar getpeername function does.

Closes #391

georgeto commented 2 years ago

I guess my only question would be is there any circumstance where this would be a breaking change for existing usage?

Also when used as parameter in string.format().

alerque commented 2 years ago

Also when used as parameter in string.format().

As far as I know all supported versions of Lua will cast a number to a string when used with string.format():

local port = 80
print(string.format("example.com:%s", port))

Likewise string concatenation works

print("example.com:"..port)

So far I haven't come up with a likely situation that would break except equality comparison

return port == "80"

Unfortunately that kind of test is likely to exist in the wild making this a potential breaking change.

What would be the harm in changing the documentation to document that we return a string and just leaving it?

siffiejoe commented 2 years ago

The string.format snippet would fail in Lua 5.1. I think it is definitely a breaking change, but it is worth fixing nevertheless. The code change looks good to me.

alerque commented 2 years ago

The string.format snippet would fail in Lua 5.1.

Why do you say that?

$ lua5.1 -e 'port = 6; print(string.format("example.com:%s", port))'
example.com:6

I think it is definitely a breaking change, but it is worth fixing nevertheless. The code change looks good to me.

Fair enough, thanks for the review.

I'll probably hold of on merging this for a little bit until I sort out where we are at with other minor changes that maybe should be released before we send out a breaking one.

siffiejoe commented 2 years ago

The string.format snippet would fail in Lua 5.1.

Why do you say that?

$ lua5.1 -e 'port = 6; print(string.format("example.com:%s", port))'
example.com:6

My bad. I just remembered that Lua 5.1 uses luaL_checklstring instead of luaL_tolstring, but it only makes a difference for values with a __tostring metamethod, not for plain numbers.

alerque commented 2 years ago

Yes. I believe it also makes a difference for nil values, just not for numbers.