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

Cast root to str to allow e.g. a pathlib.Path object to be used #358

Merged
merged 5 commits into from
Mar 2, 2024

Conversation

AdamOrmondroyd
Copy link
Collaborator

@AdamOrmondroyd AdamOrmondroyd commented Dec 5, 2023

Description

A quick one: I'm a fan of pathlib.Path, which takes care of / vs \ on unix/dos derived OSs, however, it cannot be used as the read_chains root argument since + is not defined for a Path, so I just cast it to string.

This PR puts the cast into read_chains. I have left all the other reading functions unmodified since they aren't intended to be invoked directly.

Fixes #357

Checklist:

  • I have performed a self-review of my own code
  • My code is PEP8 compliant (flake8 anesthetic tests)
  • My code contains compliant docstrings (pydocstyle --convention=numpy anesthetic)
  • New and existing unit tests pass locally with my changes (python -m pytest)
  • I have added tests that prove my fix is effective or that my feature works
  • I have appropriately incremented the semantic version number in both README.rst and anesthetic/_version.py

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e5b3bdf) to head (33a473f).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #358   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         2978      2979    +1     
=========================================
+ Hits          2978      2979    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AdamOrmondroyd AdamOrmondroyd changed the title Case root to str to allow e.g. a pathlib.Path object to be used Cast root to str to allow e.g. a pathlib.Path object to be used Dec 5, 2023
Copy link
Collaborator

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

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

This seems a pretty reasonable extension (version number and readme need bumping, but will re-approve

@AdamOrmondroyd
Copy link
Collaborator Author

will do once #356 is through

@AdamOrmondroyd
Copy link
Collaborator Author

Will be a nice opportunity to test that bump_version actually works on osx after #356 is merged

@appetrosyan
Copy link
Collaborator

I could test OS X compat

@AdamOrmondroyd
Copy link
Collaborator Author

If you like, but I'm also on Mac hence #356 in the first place

@AdamOrmondroyd
Copy link
Collaborator Author

AdamOrmondroyd commented Mar 2, 2024

If you like, but I'm also on Mac hence #356 in the first place

Will was too fast 😢 was confused why my branch had diverged in seconds

@williamjameshandley
Copy link
Collaborator

Ah, apologies for stealing your fun! The bump version isn't much use in a merge resolution anyway. We'll have to wait until a new PR before trying #356 out on macOS

@AdamOrmondroyd AdamOrmondroyd merged commit 3ea992c into handley-lab:master Mar 2, 2024
20 of 22 checks passed
@AdamOrmondroyd AdamOrmondroyd deleted the path branch March 2, 2024 00:35
@AdamOrmondroyd
Copy link
Collaborator Author

dw I'll try it out on #359

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.

Allow pathlib.Path objects to be used as root
3 participants