Skip to content

Conversation

@tesch1
Copy link

@tesch1 tesch1 commented Aug 5, 2016

maybe this will be useful for someone

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.8%) to 60.674% when pulling 3eebb50 on tesch1:ymodem into 86d3fef on tehmaze:master.

@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Coverage decreased (-6.6%) to 60.955% when pulling 3eebb50 on tesch1:ymodem into 86d3fef on tehmaze:master.

@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage decreased (-6.7%) to 60.784% when pulling d6e97c3 on tesch1:ymodem into 86d3fef on tehmaze:master.

@coveralls
Copy link

coveralls commented Aug 16, 2016

Coverage Status

Coverage decreased (-6.7%) to 60.784% when pulling d26b492 on tesch1:ymodem into 86d3fef on tehmaze:master.

@jquast
Copy link
Collaborator

jquast commented Aug 20, 2016

Wanted to drop in and thank you for the contribution, @tesch1.

We were always aware that ymodem is a small expansion of the xmodem protocol code,

but this small difference of multiple filenames does not pair with our original XMODEM class API very well. I think you have seen some of this difficulty in the code you have written.

We'll accept this code eventually, but we'll need an API change to do so, and this will have be done with some consideration. I would also need a pairing recv() implementation, and tests that provide coverage.

A recv() implementation alone is difficult, because for ymodem, the developer needs to receive the filename, and to receive many multiple files. How could our existing API indicate these batches to the caller?

@tesch1
Copy link
Author

tesch1 commented Aug 20, 2016

How would you suggest doing the send() api?

@coveralls
Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-6.8%) to 60.955% when pulling bdb9258 on tesch1:ymodem into aed6428 on tehmaze:master.

@jquast
Copy link
Collaborator

jquast commented Jun 10, 2023

Sorry to take so long (7 years!), but I'll certainly consider merging this ymodem send-only support for the moment. It was the missing of recv(), and any automatic tests, that prevented this from merging for previous release, but anyway maybe I can afford the time in the coming weeks.

@renbingcheng
Copy link

Just now, I submitted a new PR (#59) to add YMODEM protocol support. Only after pushing did I realize that #24 had already implemented YMODEM support back in 2016... and now it's 2025. Could this be the longest file transfer test in open-source history? @jquast @tehmaze @tesch1

@jquast
Copy link
Collaborator

jquast commented Apr 14, 2025

Thank you @renbingcheng, sorry that you duplicated this work.

Note that there is a YMODEM library fork of this one by @jackjack821, it supports receiving files and has documentation, https://github.com/jackjack821/ymodem-python -- although I don't think it supports sending multiple files like this PR #24 does. I do believe sending and receiving multiple files is a primary requirement for "Y-MODEM support"..

This PR #24 is more complete and so it is preferred over #59. It contains some documentation and support for multiple files while #59 only supports sending one file at a time, I don't even really mind that the comments are not in english, but that there is not any documentation, in the README or API "Docstrings" to indicate how to send a YMODEM file.


In any case, over the last nine years my attitude on publishing quality open source code with automatic test coverage is deteriorating. If I have to chose one PR, I chose this one, and I can merge and release it without tests. @renbingcheng if could you test it manually and let me know if it is successful, then I'll then merge this #24 and release to pypi.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants