Parsing compatibility of the FTP LIST command for Windows servers

[atollk]

Type of changes

• Bug fix
• New feature
• Documentation / docstrings
• Tests
• Other

Checklist

• I've run the latest black with default args on new code.
• I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
• I've added tests for new code.
• I accept that @willmcgugan may be pedantic in the code review.

Description

Extended parsing compatibility of the FTP LIST command for Windows servers. Fixes #438 .
The parsing process now properly supports 24-hour time format, both with and without leading zeros. Unit tests were adapted accordingly.

[lurch]

If you're feeling generous, perhaps https://pydoc.net/grizzled/0.9.4/grizzled.net.ftp.parse/ and https://github.com/stevemayne/pyftpparser would appreciate similar patches? 🙂

[atollk]

I created an issue for pyftpparser; I'll have to see whether I'll also do the PR there.
Regarding grizzled, the Github repo seems to be out of sync with that documentation, so it seems like it would be much more work to figure out, where the code would even go.

[atollk]

Hey, any update on this? The PR seems to be ready to merge.

[lurch]

@atollk I know it's not in code that you've modified in this PR, but I think it might be worth making these tweaks to the _parse_time function:

--- a/fs/_ftp_parse.py
+++ b/fs/_ftp_parse.py
@@ -85,12 +85,12 @@ def parse_line(line):
 
 
 def _parse_time(t, formats):
-    t = " ".join(token.strip() for token in t.lower().split(" "))
-
     _t = None
     for frmt in formats:
         try:
             _t = time.strptime(t, frmt)
+            if _t:
+                break
         except ValueError:
             continue
     if not _t:

1. Due to the way that split and join work, all the removed line effectively does is t = t.lower() - but that isn't necessary as Python's strptime is equally happy with mixed-case strings (and it copes with multiple spaces too, which this line wasn't removing in any case!)
2. The second change makes it more explicit that it stops looping through formats once it finds one that works (rather than implicitly assuming that there'll only be one format that won't raise a ValueError)

[lurch]

@atollk I've looked into it a bit further, and it seems like strptime doesn't require the leading '0' to be added to the %H format:

>>> import time
>>> time.strptime("11-02-17    04:54".strip(), "%d-%m-%y %H:%M")
time.struct_time(tm_year=2017, tm_mon=2, tm_mday=11, tm_hour=4, tm_min=54, tm_sec=0, tm_wday=5, tm_yday=42, tm_isdst=-1)
>>> time.strptime("11-02-17    4:54".strip(), "%d-%m-%y %H:%M")
time.struct_time(tm_year=2017, tm_mon=2, tm_mday=11, tm_hour=4, tm_min=54, tm_sec=0, tm_wday=5, tm_yday=42, tm_isdst=-1)

which makes your mtime = "0" + mtime unnecessary?

In fact, with this change:

@@ -145,12 +145,8 @@ def decode_linux(line, match):
     return raw_info
 
 
-def _decode_windowsnt_time(mdate, mtime):
-    if len(mtime.split(":")[0]) == 1:
-        mtime = "0" + mtime
-    return _parse_time(
-        mdate + " " + mtime, formats=["%d-%m-%y %I:%M%p", "%d-%m-%y %H:%M"]
-    )
+def _decode_windowsnt_time(mtime):
+    return _parse_time(mtime, formats=["%d-%m-%y %I:%M%p", "%d-%m-%y %H:%M"])
 
 
 def decode_windowsnt(line, match):
@@ -179,7 +170,7 @@ def decode_windowsnt(line, match):
         raw_info["details"]["size"] = int(match.group("size"))
 
     modified = _decode_windowsnt_time(
-        match.group("modified_date"), match.group("modified_time")
+        match.group("modified_date") + " " + match.group("modified_time")
     )
     if modified is not None:
         raw_info["details"]["modified"] = modified

all the unit-tests still pass, and it has the benefit of making the _decode_windowsnt_time function signature more similar to the _decode_linux_time function signature?
Feel free to ignore this comment if you feel I'm being too picky!

[atollk]

and it seems like strptime doesn't require the leading '0' to be added to the %H format

That's a fair point. I just followed the documentation there without actually trying it.

[atollk]

Hi. Any update on this?

[lurch]

Oooh, I've just learned about the for ... else ... syntax in Python which I wasn't aware of before 🙂

[lurch]

LGTM 👍 ping @althonos

[atollk]

Happy new year :)
Any update on this? @althonos

[althonos]

LGTM @atollk ! We should be able to get this in the next debug release (once we synchronize on how we want to manage releases there). Cheers!