-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Ensure demo example works without error #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your PRs !
Could you sync your fork with the current main as it includes modifications that you suggested in other PR ?
I made small comments
I'm not entirely sure how to handle all the Since this isn't what actually happens when you run pytest-copie, we should change them. I count 4 places where this is written in the docs:
For (2) and (3) I can do this, because we have prior knowledge about what the contents of the assert result.project_dir.is_dir()
with open(result.project_dir/"README.rst") as f:
assert f.readline() == "foobar\n" # or "helloworld\n" For (1), we don't have any prior knowledge of the template contents, so we can't do this. It might be best to just remove that line. That might not be a great solution, but it is better than keeping the inaccurate info. For (4), I'm not sure. I haven't used custom templates yet. I'll have to try it out and see what happens. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
==========================================
- Coverage 98.59% 96.00% -2.60%
==========================================
Files 2 2
Lines 71 75 +4
==========================================
+ Hits 70 72 +2
- Misses 1 3 +2 ☔ View full report in Codecov by Sentry. |
Nice catch - I totally assumed I was appending to that file instead of overwriting it, thank you! I think it should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long review time, my comments were on stale for 2 weeks. the checks are passing so I'll merge this one. Thanks for your contribution !
Closes #48