-
Notifications
You must be signed in to change notification settings - Fork 21
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
Path: more compatibility with pathlib.Path #331
Conversation
6a1bf58
to
4f47abd
Compare
4f47abd
to
e102dc4
Compare
e102dc4
to
d5be93a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the implementation seems to be much better than original one. I appreceate you so much and want to merge this, given a few nitpicks addressed.
return fp.read() | ||
@classmethod | ||
def cwd(cls: Type[SelfPathType]) -> SelfPathType: | ||
raise _not_supported() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some FSes support .cwd including pfio.v2.Local
. I want underlying FSes handle this method call (if it doesn't implement cwd, then let it throw unsupported exception. Oh, it has been never implemented. Nevermind..
fs: FS, | ||
scheme: Optional[str] = None, | ||
) -> None: | ||
if isinstance(fs, Local): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: do you @yhrkw have a plan to support custom scheme in near future? Currently, pfio.v2.FS
does not have .scheme
property, but it'd be worth trying adding the feature to FS class and using it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm currently working on it. I will send a PR after merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
This PR improves compatibility between
pfio.v2.pathlib.Path
andpathlib.Path
.The key differences between this PR and current impl./
pathlib.Path
as follows.NotImplementedError
because they require unsupported features ofFS
scheme
property foras_uri
(will move toFS
)as_uri
returnsfile
,s3
,hdfs
or custom scheme URI dependent onfs
andscheme
stat
returnspfio.v2.FileStat
instead ofos.stat_result