Skip to content

Conversation

leafo
Copy link
Contributor

@leafo leafo commented Nov 11, 2016

in luasocket you can pass a numeric value to a header and it sends it correctly as a string. With your compat library it will raise an error:

moon: ...leafo/.luarocks/share/lua/5.1/http/h1_connection.lua:399: field value invalid
stack traceback:
	[C]: in function 'assert'
	...leafo/.luarocks/share/lua/5.1/http/h1_connection.lua:399: in function 'write_header'
	...ome/leafo/.luarocks/share/lua/5.1/http/h1_stream.lua:689: in function 'write_headers'

I don't know where the appropriate place to fix this is, do you want to have the core library cast to string, or just have this fix as part of the compatibility layer?

In any case, I made a simple fix in the compatibility layer, but feel free to do your own solution.

@coveralls
Copy link

coveralls commented Nov 11, 2016

Coverage Status

Coverage remained the same at 86.199% when pulling ac1d207 on leafo:master into 5e49a29 on daurnimator:master.

@daurnimator
Copy link
Owner

do you want to have the core library cast to string

No.

have this fix as part of the compatibility layer

this seems like the correct place.


However, I assume that the old code you're referring to is relying on implicit number=>string coercion? Keep in mind that the default "style" of numbers was never defined. In lua 5.3 floats will get a '.0'.

https://github.com/diegonehab/luasocket/blob/b9f6fd215a8f522733caedcb4d6d6b0c6e55103e/src/http.lua#L134

        h = (canonic[f] or f) .. ": " .. v .. "\r\n" .. h

Instead of calling tostring I'd like the exact same behaviour, i.e. use the .. operator. e.g.

   field = field .. "" -- coercion from number to string. the concat operator is right associative
   req.headers:append(name:lower(), field)

Also, please use my commit message style:

path(s)/affected: What I changed or fixed

Commentary on why this implementation was favoured over others.

Reference to issue #

@daurnimator
Copy link
Owner

Oh, and could you write a test to go with this PR?

@daurnimator daurnimator merged commit ac1d207 into daurnimator:master Nov 15, 2016
daurnimator added a commit that referenced this pull request Nov 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants