-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
fix: Missing passing of skip_tasks
flag
#1688
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1688 +/- ##
==========================================
+ Coverage 97.59% 97.72% +0.12%
==========================================
Files 49 49
Lines 4993 5002 +9
==========================================
+ Hits 4873 4888 +15
+ Misses 120 114 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks for discovering and fixing this bug, @arnaubt! 🙏 LGTM! 🎉
Just one request. It would be great to have a test, e.g. similar to:
Lines 94 to 113 in 3f89b71
@pytest.mark.parametrize("unsafe", [False, True]) | |
def test_copy_cli( | |
tmp_path_factory: pytest.TempPathFactory, | |
capsys: pytest.CaptureFixture[str], | |
unsafe: bool, | |
) -> None: | |
src, dst = map(tmp_path_factory.mktemp, ["src", "dst"]) | |
build_file_tree( | |
{(src / "copier.yaml"): yaml.safe_dump({"_tasks": ["touch task.txt"]})} | |
) | |
_, retcode = CopierApp.run( | |
["copier", "copy", *(["--UNSAFE"] if unsafe else []), str(src), str(dst)], | |
exit=False, | |
) | |
if unsafe: | |
assert retcode == 0 | |
else: | |
assert retcode == 4 | |
_, err = capsys.readouterr() | |
assert "Template uses potentially unsafe feature: tasks." in err |
Would you mind adding one?
@sisp Sure enough, test added! Let me know if it's good enough. |
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.
LGTM, just some minor requests. Sorry that I was a bit imprecise, I was thinking to add the test in tests/test_tasks.py
perhaps below
Lines 69 to 82 in 3f89b71
def test_copy_skip_tasks(template_path: str, tmp_path: Path) -> None: | |
copier.run_copy( | |
template_path, | |
tmp_path, | |
quiet=True, | |
defaults=True, | |
overwrite=True, | |
skip_tasks=True, | |
) | |
assert not (tmp_path / "hello").exists() | |
assert not (tmp_path / "hello").is_dir() | |
assert not (tmp_path / "hello" / "world").exists() | |
assert not (tmp_path / "bye").is_file() | |
assert not (tmp_path / "pyfile").is_file() |
Could you please move it there?
Test moved and suggestions applied, thanks! |
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.
LGTM! 🎉 Thanks again, @arnaubt! 🙏
@sisp Great! When do you expect this to be released? |
Thank you very much! 🙏 |
Couldn't get the
--skip-flags
option working until I passed the option to the worker.