Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Respectos.Pathlike#2086

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
George-Ogden wants to merge18 commits intogitpython-developers:main
base:main
Choose a base branch
Loading
fromGeorge-Ogden:true-pathlike

Conversation

@George-Ogden
Copy link
Contributor

Fixes#2085
Replaces instances ofstr(path) withpath.__fspath__() for more general usage.
I also moved the clone tests into a separate file that existed before but only contained a single test.

@George-Ogden
Copy link
ContributorAuthor

Also, I notice you skip mypy because it produces errors. Why not pin the version and increment it periodically?

@ByronByron requested a review fromCopilotNovember 28, 2025 06:44
Copilot finished reviewing on behalf ofByronNovember 28, 2025 06:45
@Byron
Copy link
Member

That's a great incentive, thanks so much! Using__fspath__() seems like it's using a private API, so it looks strange to my untrained eye. But if it's correct, I guess it's worth doing anyway?

Also, I notice you skip mypy because it produces errors. Why not pin the version and increment it periodically?

I don't see why this wouldn't work, great idea! Please feel free to set this up in your next PR :).

@ByronByron requested review fromCopilot and removed request forCopilotNovember 28, 2025 07:00
Copilot finished reviewing on behalf ofByronNovember 28, 2025 07:00
@Byron
Copy link
Member

It seems that I won't get my copilot review here, so I wonder why you'd not be callingos.fspath(path) instead?

@George-Ogden
Copy link
ContributorAuthor

It seems that I won't get my copilot review here, so I wonder why you'd not be callingos.fspath(path) instead?
I agree that's much nicer and also gets rid of theif not isinstance(path, str) checks.

@George-Ogden
Copy link
ContributorAuthor

I don't see why this wouldn't work, great idea! Please feel free to set this up in your next PR :).

Sure thing

Copy link

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Member

@ByronByron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This looks better indeed.
Something I think will be desirable is to actually add tests for non-decodable paths for the functions/types that are affected. Otherwise, how do we know it's working?

Feel free to use AI for that, it's quite good at this usually.

The idea is to prove that the changes actually make something possible that wasn't possible before.

# When pathlib.Path or other class-based path is passed
ifnotisinstance(path,str):
path=str(path)
url=os.fspath(url)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

URL is not a path though.

Copy link
ContributorAuthor

@George-OgdenGeorge-OgdenNov 29, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Here,url can be a path if you're cloning a local repo, and if it's notos.fspath will leave strings alone.

@ByronByron requested a review fromCopilotNovember 29, 2025 10:00
Copilot finished reviewing on behalf ofByronNovember 29, 2025 10:02
Copy link

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
George-Ogdenand others added2 commitsNovember 29, 2025 11:57
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@George-Ogden
Copy link
ContributorAuthor

I've added a few tests, but lots of the calls work on internal APIs, so they won't make much difference. In these cases, callingos.fspath is still better as it makes the intention clearer. I also made a tool to check for redundant uses and the only one isgit/util.py:420.

Copy link
Member

@ByronByron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That's great, thanks a lot!

What should be shown in the tests is that it can now handle filepaths that don't decode with the standard encoding.

Candidate tests are:

  • clone from a filesystem path
  • index.add with a strange path
  • and probably more - you could intentionally break a piece of code that changedfspath and see which tests fail, then it's clear where to add a test with a 'special' path.

Thanks for making this happen, and thanks for your understanding - we can't just make changes hoping it will work or doesn't make things work, but there must be real evidence that this is desirable. And we can only have that with tests that fail without this change.

@Byron
Copy link
Member

I am putting the PR back to draft until there are tests that use the new "can handle paths encoding independently from the runtime encoding" to prove the changes are effective. Thanks again.

@George-Ogden
Copy link
ContributorAuthor

Sorry, I meant to add this comment with the changes I made yesterday:
I've gone through all the changes and made sure that they apply (and removed the ones that didn't). As a result, there are more tests for submodules and index, as well as the existing ones I added.

For example, theclone_from_pathlike test means that all thefs.path conversions inRepo.base are required and removing them will cause that test to fail. Is that what you mean by "can handle paths encoding independently from the runtime encoding"? If not, I can add tests that will do that.

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

Reviewers

@ByronByronByron requested changes

Copilot code reviewCopilotCopilot left review comments

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pathlike type hint is not respected

2 participants

@George-Ogden@Byron
[8]ページ先頭
©2009-2025 Movatter.jp