Skip to content

Support for Windows NT FTP servers#227

Merged
willmcgugan merged 2 commits intoPyFilesystem:masterfrom
sspross:support-windowsnt-ftpserver
Nov 3, 2018
Merged

Support for Windows NT FTP servers#227
willmcgugan merged 2 commits intoPyFilesystem:masterfrom
sspross:support-windowsnt-ftpserver

Conversation

@sspross
Copy link
Copy Markdown
Contributor

@sspross sspross commented Nov 2, 2018

We had some problems trying to connect to a Windows NT FTP server. FTP LIST lines look totally different from the linux ones, therefore we needed another regex parser and decoder in the _ftp_parse.py file. I tested it against this server and our requirements (list directories, create directories, create files) work fine. But let me know if this PR doesn't match your style or if I have to do something else/different. Best regards!

Support for Windows NT lines ftpfs.py:_read_dir():

[
    '11-02-18  02:00PM       <DIR>          asdf',
    '11-02-18  01:50PM       <DIR>          data',
    '11-01-18  01:41PM       <DIR>          doc',
    '11-02-18  02:12PM       <DIR>          images',
    '11-02-18  03:33PM                 9276 logo.gif'
]

@willmcgugan
Copy link
Copy Markdown
Member

Looks great! Any chance of a unit test?

@sspross
Copy link
Copy Markdown
Contributor Author

sspross commented Nov 3, 2018

Sure, I just added a unit test and fixed some of the CI warnings as well. But first, because of my _parse_time change, the unit tests failed locally. But the CI was happy here... is there something wrong with the CI reporting into github checks?

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.7%) to 99.049% when pulling cbd2234 on sspross:support-windowsnt-ftpserver into 5643bad on PyFilesystem:master.

@willmcgugan
Copy link
Copy Markdown
Member

Thanks. I'm not sure why CI was reporting an error there. I'll look in to that.

@willmcgugan willmcgugan merged commit e1f5fbb into PyFilesystem:master Nov 3, 2018
@sspross
Copy link
Copy Markdown
Contributor Author

sspross commented Nov 3, 2018

Thanks for merging! Just to clarify: The problem was, that the CI was NOT reporting an error here in the CI github hook. And last question: Do you will create a minor version bump for this update? Best Regards, Silvan

@willmcgugan
Copy link
Copy Markdown
Member

Yeah, I don't know why the CI error wasn't showing up. It certainly used to.

Will do a minor update soon. Need to review some other fixes.

@willmcgugan
Copy link
Copy Markdown
Member

Fix is in v2.1.2

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants