Skip to content

Conversation

@bakhtos
Copy link

@bakhtos bakhtos commented Oct 17, 2025

Fixes #315

Do not force paths to be strings. Most functions now manipulate PathLike objects. Forced conversions to string are removed.
Conversion to string is only done when checking URL.

Since the user will only interact directly with Repository, the check that path_to_repo is of appropriate type is moved there from Conf.

Minimum Python version is now 3.6 to support os.PathLike

@testpulseio
Copy link

testpulseio bot commented Oct 17, 2025

TestPulse report

Test execution

⚠️ There were 1 failing tests!

Files without coverage

⚠️ Some files changed in the PR are not covered by tests, careful!

  • setup.py

Coverage information

No changes in coverage, nice! 🎉

@bakhtos
Copy link
Author

bakhtos commented Oct 17, 2025

The only test that failed is test_commit.test_metrics_python also on my system, but it does not seem to be related to the change.

Comment on lines +119 to +122
if isinstance(path_to_repo, list):
path_to_repos = [os.fspath(path) for path in path_to_repo]
else:
path_to_repos = [os.fspath(path_to_repo)]
Copy link
Owner

@ishepard ishepard Oct 18, 2025

Choose a reason for hiding this comment

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

Why don't we just transform everything in Path here instead of leaving it as strings? We could just transform it to string later when we need it, like in _is_remote. Wdyt?
I feel like it would be much easier to read and play with just Path, rather than a mix. But let me know since you worked on it 😄

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I think this was the first change that I coded, when I thought I will just keep using strings internally. Then I realised to used PathLike also internally. So I can make it that Repository always converts init parameters to Path objects, and all internal functions assume they are handling Path objects as well.

@ishepard
Copy link
Owner

Yeah the test is not your fault. I fixed it now, can you please rebase your branch to master?

Also, can you please add a couple of tests to cover all the changes? Thanks!

@bakhtos
Copy link
Author

bakhtos commented Oct 20, 2025

Yeah the test is not your fault. I fixed it now, can you please rebase your branch to master?

Also, can you please add a couple of tests to cover all the changes? Thanks!

Yes, I will use the fixed tests and also duplicate some existing tests which load the repo so that they use Path objects instead of str as input.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Repository constructor should accept Path objects

2 participants