Skip to content
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

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

yhrkw
Copy link
Contributor

@yhrkw yhrkw commented Jan 12, 2024

This PR improves compatibility between pfio.v2.pathlib.Path and pathlib.Path.

The key differences between this PR and current impl./pathlib.Path as follows.

  • many methods raise NotImplementedError because they require unsupported features of FS
  • provides scheme property for as_uri (will move to FS)
  • as_uri returns file, s3, hdfs or custom scheme URI dependent on fs and scheme
  • stat returns pfio.v2.FileStat instead of os.stat_result

@yhrkw yhrkw force-pushed the pathlib-compatible branch 2 times, most recently from 6a1bf58 to 4f47abd Compare January 15, 2024 09:33
@yhrkw yhrkw marked this pull request as draft March 28, 2024 07:03
@yhrkw yhrkw marked this pull request as ready for review March 28, 2024 07:13
@kuenishi kuenishi self-requested a review March 29, 2024 05:05
@kuenishi kuenishi added this to the 2.8.0 milestone Apr 2, 2024
Copy link
Member

@kuenishi kuenishi left a 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()
Copy link
Member

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..

pfio/v2/pathlib.py Outdated Show resolved Hide resolved
pfio/v2/pathlib.py Show resolved Hide resolved
pfio/v2/pathlib.py Outdated Show resolved Hide resolved
fs: FS,
scheme: Optional[str] = None,
) -> None:
if isinstance(fs, Local):
Copy link
Member

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.

Copy link
Contributor Author

@yhrkw yhrkw Apr 16, 2024

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.

@yhrkw yhrkw requested a review from kuenishi April 16, 2024 06:59
Copy link
Member

@kuenishi kuenishi left a 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!

@kuenishi kuenishi merged commit e3c480b into pfnet:master Apr 16, 2024
6 checks passed
@kuenishi kuenishi added the cat:enhancement Implementation that does not break interfaces. label Apr 16, 2024
@yhrkw yhrkw deleted the pathlib-compatible branch April 17, 2024 00:52
@yhrkw yhrkw mentioned this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement Implementation that does not break interfaces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants