Skip to content

Fix #124: Support pathlib.Path in loadStructure and add test #129

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

Closed
wants to merge 2 commits into from

Conversation

kwagoner94
Copy link
Contributor

This PR resolves #124 by adding support for pathlib.Path inputs in loadStructure.

Summary:

  • Adds os.fspath(filename) to allow str, pathlib.Path, and os.PathLike file inputs.
  • Updates the docstring to clarify accepted types.
  • Adds a new unit test (test_cif_pathlib) to verify that loadStructure accepts Path objects.
  • Confirmed functionality with both automated and manual tests.

Note:

Some unrelated tests are failing due to YappsSyntaxError now being raised by PyCifRW instead of StructureFormatError. These issues predate this change and are unrelated to the fix provided here.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Thanks so much for this @kwagoner94

Couple of comments

  1. is it possible you don't have pre-commit running locally? It looks as if pre-commit had to make changes in the CI which usually indicates that. Please check the scikit-package docs, but you are missing a pre-commit install command that must be run in your local repo.
  2. Things are in a bit of a precarious state because CI is not running because we have migrated our release scripts in the latest version of scikit-package. We must merge a fix to skpkg: use scikit-package v0.1.0 to restandarlize package #126 before we can merge anything else. Do you want to take that one? You will have to follow the instructions for migrating a package to scikit-package level 5

Again, thanks so much for the fix. I see it was done carefully and I appreciate the detailed info you shared in the comments etc..... excellent! I can tell that things are going to go well!

@sbillinge
Copy link
Contributor

@kwagoner94 you mentioned failing tests coming from an update in pyCIFrw. Please could you create an issue for someone to fix that? Failing tests are usually a high priority so we want to address that also with some urgency. We may want to merge that before we merge this also, but after #123

@kwagoner94
Copy link
Contributor Author

@sbillinge I’ve created a new issue to track the YappsSyntaxError test failures.

As for pre-commit, you’re right — I hadn’t set it up properly. It’s now installed and configured locally. Let me know if you'd prefer that I push a new commit with the pre-commit fixes now, or wait until #126 is merged and then rebase.

I’m also happy to take on #126 and help migrate the repo to scikit-package level 5.

@sbillinge
Copy link
Contributor

@sbillinge I’ve created a new issue to track the YappsSyntaxError test failures.

👍

As for pre-commit, you’re right — I hadn’t set it up properly. It’s now installed and configured locally. Let me know if you'd prefer that I push a new commit with the pre-commit fixes now, or wait until #126 is merged and then rebase.

I think this PR is pretty clean so no worries. You will have to sync your local from your fork if there are any more commits you have to push (no worries if not). We don't rebase btw, we want to keep a linear commit history.

I’m also happy to take on #126 and help migrate the repo to scikit-package level 5.

Yes, please! Could you do that?

@kwagoner94 kwagoner94 closed this by deleting the head repository Jun 6, 2025
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.

Check if loadStructure and other file loading functions accept pathlib.Path
2 participants