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

Socker:receive("l") without asterisk, L option #294

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mbartlett21
Copy link

This pull request allows users to call socket:receive() without the parameter having to start with an asterisk. It also adds the L option, which includes the end of line marker.

The TCP Receive code now uses a switch statement and allows dropping the * at the start of the string, like Lua's io library
@Tieske
Copy link
Member

Tieske commented Mar 22, 2022

Is this safe to use on different systems? unix vs windows, with single charcter lineends versus windows CrLf ?

@Tieske
Copy link
Member

Tieske commented Mar 22, 2022

btw the "L" option is useful to be in line with newer Lua versions

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test and documentation updates are missing, mind adding those?

@mbartlett21
Copy link
Author

Is this safe to use on different systems? unix vs windows, with single charcter lineends versus windows CrLf ?

@Tieske

Yes, it is. It currently just drops any CRs.

@mbartlett21
Copy link
Author

mbartlett21 commented Mar 23, 2022

test and documentation updates are missing, mind adding those?

@Tieske
For the documentation, do I remove mentions of it with the asterisk, seeing as that seems to be what happened with file:read()?

@Tieske
Copy link
Member

Tieske commented Mar 23, 2022

Is this safe to use on different systems? unix vs windows, with single charcter lineends versus windows CrLf ?

@Tieske

Yes, it is. It currently just drops any CRs.

don't windows users then end up with a lingering LF? I'm not sure what the Lua 5.3 behaviour is, but I think it should mimic that. As it would be the "least surprising" option.

@alerque
Copy link
Member

alerque commented Mar 23, 2022

I haven't reviewed this in detail, just noting my impression that it should be merged after the next (safe-harbor) release tag.

@mbartlett21
Copy link
Author

Is this safe to use on different systems? unix vs windows, with single charcter lineends versus windows CrLf ?

@Tieske
Yes, it is. It currently just drops any CRs.

don't windows users then end up with a lingering LF? I'm not sure what the Lua 5.3 behaviour is, but I think it should mimic that. As it would be the "least surprising" option.

Lua, for file:read() seems to just let CRs through. Is that what you want me to do here?

@Tieske
Copy link
Member

Tieske commented Mar 24, 2022

Lua, for file:read() seems to just let CRs through. Is that what you want me to do here?

What did you test? I tested "*L" (on OSX) and it seems to cut on LF (10) (a single CR is ignored)

my test code:

print(_VERSION)
local CR = string.char(13)
local LF = string.char(10)

local function writefile(name, content)
  local f = io.open(name, "wb")
  f:write(content)
  f:close()
end

local function readfile(name, mode)
  local f = io.open(name, "r")
  while f do
    local l = f:read(mode)
    if l then
      local le = l:gsub(CR, "+CR"):gsub(LF, "+LF")
      if le:sub(1,1) == "+" then
        le = le:sub(2,-1)
      end
      print(#l, le)
    else
      f:close()
      f = nil
    end
  end
end

local lines = {               -- "*L" result:
  "1234567890"..LF,           -- 1 line, 11 bytes
  "1234567890"..LF..CR..LF,   -- 2 lines, 11 bytes + 2 bytes
  "1234567890"..CR..LF,       -- 1 line, 12 bytes
  "1234567890"..CR..LF..LF,   -- 2 lines, 12 bytes + 1 byte
  "1234567890"                -- 1 line, 10 bytes
}
lines = table.concat(lines)

writefile("test.txt", lines)
for _, mode in ipairs { "*l", "*L" } do
  print("\nTesting mode: "..mode)
  readfile("test.txt", mode)
end

Output:

Lua 5.3

Testing mode: *l
10	1234567890
10	1234567890
1	CR
11	1234567890+CR
11	1234567890+CR
0	
10	1234567890

Testing mode: *L
11	1234567890+LF
11	1234567890+LF
2	CR+LF
12	1234567890+CR+LF
12	1234567890+CR+LF
1	LF
10	1234567890
Program completed in 0.16 seconds (pid: 86623).

So looks to me like "*l" is platform dependent, whereas "*L" just cust right after the "LF", and is platform independent. I'll try and test on Windows as well.

EDIT: writing the file; added the "B" for binary mode

@Tieske
Copy link
Member

Tieske commented Mar 24, 2022

on Windows I get;

Lua 5.3

Testing mode: *l
10	1234567890
10	1234567890
0	
10	1234567890
10	1234567890
0	
10	1234567890

Testing mode: *L
11	1234567890+LF
11	1234567890+LF
1	LF
11	1234567890+LF
11	1234567890+LF
1	LF
10	1234567890
Program completed in 0.25 seconds (pid: 18632).

The "*l" option converts the line endings, which differs from the Unix one. At the same time the "*L" option, also differs because it retains the line endings, but only AFTER conversion.

EDIT: updated this post after adding the "B" to the file being written in the test code

@Tieske
Copy link
Member

Tieske commented Mar 27, 2022

Updated the test code and test results above, after a bug in the test code.

@Tieske
Copy link
Member

Tieske commented Mar 27, 2022

The culprit seems to be the difference between "text" and "binary" mode on Windows systems, because if the file is opened for reading "binary" mode, then the Windows output is identical to the Mac output.

So I think we should assume a socket to have a "binary" format when reading, and as such should get the same results. That would be cutting the line on each "LF" character, and not dropping any preceding "CR" character. Which is what the current PR seems to be doing.

So a lot of fuss to conclude the PR looks good. But it would need some tests to prove the behavior.

@alerque
Copy link
Member

alerque commented Mar 28, 2022

On the subject of tests, I would really prefer if all new tests were written in busted instead of the current ad-hock framework. Even if it's one test at a time lets start migrating that direction when we need a new test for something or to do some substantial fix to an existing test. I'll try to get a couple basic ones started so there is a pattern to follow.

@alerque
Copy link
Member

alerque commented Mar 28, 2022

(This case of course is exacerbated by the fact that we're not current doing any CI testing on Windows at all; I'd be glad to facilitate if somebody with expertise knows how to make it go...)

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

Successfully merging this pull request may close these issues.

3 participants