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

GAM 20200325 1100 : added optional end of line parameter to readline … #95

Closed
wants to merge 2 commits into from

Conversation

@graemorrison
Copy link

graemorrison commented Mar 25, 2020

added optional eol parameter to specify end of line marker if it's not '\r\n'

graemorrison added 2 commits Mar 25, 2020
@FoamyGuy

This comment has been minimized.

Copy link
Contributor

FoamyGuy commented Mar 25, 2020

@graemorrison Thank you for this change! I will work on testing it tonight and we should be able to get it merged in soon.

@FoamyGuy FoamyGuy requested a review from adafruit/circuitpythonlibrarians Mar 25, 2020
@FoamyGuy FoamyGuy self-assigned this Mar 25, 2020
Copy link
Contributor

tannewt left a comment

Why is this needed? I'd rather not add a kwarg to readline that doesn't exist in the standard CPython readline. https://docs.python.org/3/library/io.html#io.IOBase.readline

@graemorrison

This comment has been minimized.

Copy link
Author

graemorrison commented Mar 25, 2020

it's needed because not all systems terminate a line with '\r\n' (such as unix), in fact if you look at your example in https://docs.python.org/3/library/io.html#io.IOBase.readline it actually specifies '\n' as the eol terminator and it's being modified in your version to be '\r\n'

@FoamyGuy

This comment has been minimized.

Copy link
Contributor

FoamyGuy commented Mar 25, 2020

@tannewt I think IOBase readline() is different because it's reading from the terminal. This is for socket readline() If I am understanding correctly it seems that the CPython socket module does not provide a readline() function at all, the closest things seems to be makefile: https://docs.python.org/3/library/socket.html#socket.socket.makefile which is admittedly not that close really. But would allow for iterating over the received data one line at a time I think. makefile() does have a newline parameter, though I don't see anywhere in the documentation that states specifically what it does, I would assume it determines the characters used within the file like object that it returns. I'm definitely no expert though perhaps I am misunderstanding something.

@graemorrison

This comment has been minimized.

Copy link
Author

graemorrison commented Mar 25, 2020

@tannewt - sorry, my previous response was little curt. Essentially the reason for the kwarg is to provide backwards compatibility with the existing version, hence the default being the current setting.

@tannewt

This comment has been minimized.

Copy link
Contributor

tannewt commented Mar 26, 2020

it's needed because not all systems terminate a line with '\r\n' (such as unix), in fact if you look at your example in https://docs.python.org/3/library/io.html#io.IOBase.readline it actually specifies '\n' as the eol terminator and it's being modified in your version to be '\r\n'

I understand this but don't know why it applies to ESP32SPI which is reading from the ESP only. Does the ESP pass line endings as returned by different servers?

My desire in the long-term is to remove this function because it doesn't match CPython's socket API (as @FoamyGuy points out). I'd much prefer that our socket classes are strict subset so that we can test things that build on top of it with CPython. I'll likely do this as we add Wifi support for the ESP32-S2.

So, I'd rather not change this API since it will be removed later and shouldn't be used in more places. Instead, I'd encourage you to use the recv*_into methods from socket instead (and add them if they are missing). They are much much better since they do not allocate new buffers at all.

@graemorrison

This comment has been minimized.

Copy link
Author

graemorrison commented Mar 27, 2020

it's needed because not all systems terminate a line with '\r\n' (such as unix), in fact if you look at your example in https://docs.python.org/3/library/io.html#io.IOBase.readline it actually specifies '\n' as the eol terminator and it's being modified in your version to be '\r\n'

I understand this but don't know why it applies to ESP32SPI which is reading from the ESP only. Does the ESP pass line endings as returned by different servers?

My desire in the long-term is to remove this function because it doesn't match CPython's socket API (as @FoamyGuy points out). I'd much prefer that our socket classes are strict subset so that we can test things that build on top of it with CPython. I'll likely do this as we add Wifi support for the ESP32-S2.

So, I'd rather not change this API since it will be removed later and shouldn't be used in more places. Instead, I'd encourage you to use the recv*_into methods from socket instead (and add them if they are missing). They are much much better since they do not allocate new buffers at all.

@tannewt, yes the ESP is ignorant of line endings it's simply returning the data as transmitted by the remote end of a network session, depending on the application running at the other side it could be anything - , , could even be or or just about any other kind of delimiter.

However I get your point about compatibility and not diverging from the upstream package, especially if the goal is to remove this in a near-future release.
I will look at the 'recv' routines and see what's appropriate to what I'm trying to do
thanks for the advice - I'll remove this PR

@graemorrison graemorrison deleted the graemorrison:readline-eol branch Mar 27, 2020
@tannewt

This comment has been minimized.

Copy link
Contributor

tannewt commented Mar 27, 2020

Thanks for understanding @graemorrison ! Let me know if I can help with the recv stuff!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.