Complete support for bytes paths in nt#992
Conversation
slozier
left a comment
slozier
left a comment
There was a problem hiding this comment.
I know it's still marked as draft, but here are my notes.
| foreach (object name in listdir(context, _filesystemEncoding.GetString(path.AsMemory().Span))) { | ||
| ret.AddNoLock(Bytes.Make(_filesystemEncoding.GetBytes((string)name))); |
There was a problem hiding this comment.
Use your ToFsString and ToFsBytes helpers?
There was a problem hiding this comment.
Yeah, a remnant from pre-helpers time.
| System.Text.StringBuilder sb = null; | ||
| if (!PythonOps.TryGetEnumerator(context, args, out argsEnumerator)) { | ||
| throw PythonOps.TypeError("args parameter must be sequence, not {0}", DynamicHelpers.GetPythonType(args)); | ||
| throw PythonOps.TypeError("args parameter must be sequence, not {1}", DynamicHelpers.GetPythonType(args)); |
There was a problem hiding this comment.
Not sure why this changed from {0} to {1}?
| times = kvp.Value == null ? null | ||
| : args[0] is PythonTuple pt2 ? pt2 | ||
| : throw PythonOps.TypeError("utime: 'times' must be either a 2-value tuple (atime, mtime) or None"); |
There was a problem hiding this comment.
Should args[0] here be kvp.Value? Maybe wrap the logic in a local function:
static PythonTuple GetTimes(object val) {
return val == null ? null
: val is PythonTuple pt ? pt
: throw PythonOps.TypeError("utime: 'times' must be either a 2-value tuple (atime, mtime) or None");
}I guess we could also do the length check on the tuple in there?
There was a problem hiding this comment.
OK. In general, I am still working on the keyword/default parameter handling and am musing about some generic, multipurpose mechanism that will cut though so much ceremony that is now required. The \u00F8 seems not to work anymore since Python 3 allows Unicode letters in identifiers. But perhaps I will start with something simpler, like #882.
| private static Bytes ToFsBytes(this string s) => Bytes.Make(_filesystemEncoding.GetBytes(s)); | ||
|
|
||
| private static Bytes ToFsBytes(this IBufferProtocol bp) { | ||
| // TODO: DeprecationWarning starting from Python 3.7 |
There was a problem hiding this comment.
I would be fine with adding the deprecation warning right away (if it doesn't cause testing failures).
|
Thanks for early feedback! |
|
I think this may solve some of the failures in |
|
I have kept it on hold because some things can get simplified once we have support for keyword-only arguments. But since I am preoccupied with other stuff right now, I can't say when this will be. So if it helps, this PR can be merged as is. I can always revisit it later on. |
|
Thanks. I'll merge it in as-is. Better to have incremental progress than wait indefinitely. 😄 |
2 participants
No description provided.