Skip to content

use pathlib.Path #21

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

Merged
merged 11 commits into from
Feb 20, 2023
Merged

use pathlib.Path #21

merged 11 commits into from
Feb 20, 2023

Conversation

hans-d
Copy link
Contributor

@hans-d hans-d commented Feb 18, 2023

fixes #7

@omarkohl
Copy link
Owner

Wow, thanks, that's a very thorough PR, updating the changelog and all 😍 That certainly makes my life easier 🙂 . I will have a look at the code. If you have time, could you do the following?

  • Check that all the examples in the README.md still work
  • Add a section in README.md documenting how people have to modify their code to upgrade to version 3.0.0

@hans-d
Copy link
Contributor Author

hans-d commented Feb 18, 2023

For the README.rst:

  • i will move the examples to their own files (makes life easier to test them)
  • and generate a full README.rst from the that

Additional changes are expected as well in tests (also getting rid of py.path there)

@hans-d
Copy link
Contributor Author

hans-d commented Feb 18, 2023

Updated. Also fixed an existing error in the examples.

Copy link
Owner

@omarkohl omarkohl left a comment

Choose a reason for hiding this comment

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

Thank you for the great work! Some comments inline and one more generic one:

  • What is the purpose of src/pytest_datafiles/init.py ?

Makefile Outdated
@echo "test-all Run tests on every Python version with tox"
@echo "coverage Check code coverage quickly with Python 3"
@echo "dist Package"
@echo "README.rst Generate README.rst from docs/readme"
Copy link
Owner

Choose a reason for hiding this comment

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

Please add the new make targets you created to the help text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Owner

Choose a reason for hiding this comment

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

not done 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done again

@hans-d hans-d requested a review from omarkohl February 19, 2023 14:28
Copy link
Owner

@omarkohl omarkohl left a comment

Choose a reason for hiding this comment

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

Great!

@@ -0,0 +1,20 @@
"""Example: Reference files anywhere """
Copy link
Owner

Choose a reason for hiding this comment

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

I think all the examples should be in a top-level directory examples/ rather than tests/examples/ so they are easier to find. But I can also do that once this PR is merged, it's easy enough. As you prefer.

Copy link
Owner

Choose a reason for hiding this comment

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

I would also put the explanation of the example that is in the README as a docstring inside each example.py file. This way all the relevant info is in one place. Again, I can do this after merging the PR as you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved, added text to the docstring. Also included just short fragments in the README itself to focus on the highlights.

@hans-d hans-d requested a review from omarkohl February 20, 2023 12:49
Copy link
Owner

@omarkohl omarkohl left a comment

Choose a reason for hiding this comment

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

🥳

@omarkohl omarkohl merged commit 02969ca into omarkohl:master Feb 20, 2023
@hans-d
Copy link
Contributor Author

hans-d commented Feb 20, 2023

Leaving the To create and upload a new package to you 📦

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.

py.path vs pathlib.Path
2 participants