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

Migrate from os.path to Pathlib #27

Open
ianepreston opened this issue Aug 3, 2020 · 8 comments
Open

Migrate from os.path to Pathlib #27

ianepreston opened this issue Aug 3, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@ianepreston
Copy link
Owner

Using os.path to handle files makes the implementation trickier, especially for cross platform support. Pathlib is great. Time to switch to pathlib

@ianepreston ianepreston added enhancement New feature or request good first issue Good for newcomers labels Aug 3, 2020
@ianepreston ianepreston self-assigned this Aug 3, 2020
@azimj
Copy link
Contributor

azimj commented Jan 29, 2021

Ian,

After watching the today's Data-for-Good/PyData meetup. I agree that this would be a good issue to explore the code. I've worked through the code updating to replase os.path with pathlib. I'm on Python 3.9 and your tests don't run with nox. So, I'm wondering how to run tests before I create a PR.

Thanks for the presentation and great work.

Azim

@ianepreston
Copy link
Owner Author

Hi Azim,

Thanks for your interest! There are a couple options for how to proceed. If you create a pull request with your changes then GitHub actions will run the tests remotely. If you want to test locally without nox you should just be able to call pytest directly from whatever environment you're in. Alternatively you could either set up other python versions, or wait for me to add python 3.9 support (that's pretty high on my to do list). What are you using to manage python versions on your machine and what operating system are you running?

Thanks

@azimj
Copy link
Contributor

azimj commented Jan 29, 2021

Thank you for the quick response. I am on Win-10 and have created a venv environment. My current toolchain/environment is VS-Code with Microsoft's Python extensions. I have my main Python install on my path. I will try the PyTest suggestion later today.

I don't have any Python version management work-flows and I've gone away from using third party distros (eg enthought, anaconda). Some packages such as iPython I've installed in my global Python but other packages I'm trying to keep to installing in virtual environments for a given project.

I will let you know how it goes. Expect a PR from me in the near future.

@azimj
Copy link
Contributor

azimj commented Jan 30, 2021

Well, I think I need some more time to get tests to run on my computer before I can create a PR and be sure that I haven't broken your tests. Anyway, I will update you on progress.

@ianepreston
Copy link
Owner Author

I've just merged in a fix that allows the package to support python 3.9. Hopefully this will make your testing easier!

@azimj
Copy link
Contributor

azimj commented Jan 30, 2021

I will try it. I think I'm having some issue with the tables package. I had to download binaries from Christoph Gohlke but I still have error's related to this package. I will merge your latest commit and try to run tests this weekend. Thanks

azimj referenced this issue in azimj/stats_can Feb 13, 2021
Updates code to use pathlib.Path instead of os.path
Also remove `import os`
@azimj
Copy link
Contributor

azimj commented Feb 13, 2021

Hello Ian,

Pull Request #64 address this issue. Let me know what you think.

Thanks

Azim

ianepreston added a commit that referenced this issue Feb 13, 2021
* Issue #27: Migrate from os.path to Pathlib

Updates code to use pathlib.Path instead of os.path
Also remove `import os`

* formatting

* version bump

Co-authored-by: Ian Preston <[email protected]>
@azimj
Copy link
Contributor

azimj commented Feb 14, 2021

You're welcome. happy to have contributed.

ianepreston added a commit that referenced this issue Jun 27, 2024
* Issue #27: Migrate from os.path to Pathlib

Updates code to use pathlib.Path instead of os.path
Also remove `import os`

* formatting

* version bump

Co-authored-by: Ian Preston <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants