Skip to content
This repository has been archived by the owner on Jan 7, 2024. It is now read-only.

replace all os.path references with pathlib #53

Closed
1 task
fgebhart opened this issue Jan 5, 2021 · 4 comments
Closed
1 task

replace all os.path references with pathlib #53

fgebhart opened this issue Jan 5, 2021 · 4 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@fgebhart
Copy link
Owner

fgebhart commented Jan 5, 2021

This currently seems to block workoutizer from supporting windows.

  • replace os.path with pathlib
@fgebhart fgebhart added the good first issue Good for newcomers label Jan 5, 2021
@fgebhart fgebhart added the help wanted Extra attention is needed label Jun 13, 2021
@polyvertex
Copy link
Contributor

How so? os.path is as Windows-compatible as it can be.

However I noticed improper file path manipulation in project's code, tests included. Typically path.split("/") or os.path.join(somedir, "foo/bar/")...

polyvertex added a commit to polyvertex/workoutizer that referenced this issue Sep 13, 2021
@fgebhart
Copy link
Owner Author

You are of course right, os.path is totally windows compatible.

When I wrote this issue, I simply added a windows-latest to the matrix test and realized a bunch of tests were failing because of improper paths.

I thought it would be good to streamline the usage of path handling methods to just use either of both options and since I preferred pathlib I came up with this issue.

Your approach seems also to be totally valid 👍

@polyvertex
Copy link
Contributor

I simply added a windows-latest to the matrix test and realized a bunch of tests were failing because of improper paths.

Path manipulations that were fixed in #209 is a step towards workoutizer's Windows compat I guess 😀

I thought it would be good to streamline the usage of path handling methods to just use either of both options and since I preferred pathlib I came up with this issue.

pathlib is appealing but its use might be overkill in some places. Typically when only a call to os.path.basename would do for instance. So that you do not need to instantiate a PurePath object (or derived) for a single path manipulation.

@fgebhart
Copy link
Owner Author

Closing this issue, since the original intend was to resolve improper path manipulation which blocked the windows compatibility. See #210 for ongoing migrations efforts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants