fix: delete conditionally created file when answer changes

[sisp]

I've fixed the copier update behavior to remove a conditionally created file when the answer changes such that the file would not be created anymore.

To achieve this behavior, I needed to change the update workflow a little.

Before:

1. The old template is copied into a temporary location with the answers from the answers file.
2. The new template is copied into a temporary location with the answers from the answers file. By the way, I think this is not quite correct because the questionnaire might have changed in the new template version and there is no user interaction in this step that allows answering to new/changed questions.
3. A diff is created between the output of 1. and the real project.
4. Pre-migration tasks are executed on the real project.
5. The new template is copied into the real destination with possible user interaction where necessary unless --data/--defaults flags are passed accordingly.
6. The diff from 3. is applied to the real project.
7. Files that exist in 1. but not in 2. are removed from the real project.
8. Post-migration tasks are executed on the real project.

After:

1. The old template is copied into a temporary location with the answers from the answers file.
2. Pre-migration tasks are executed on the real project. It's important to move this step up in the new workflow to support, e.g., answer file migrations. And it's also important to run this step after 1. because the old template must be copied before the pre-migration tasks have been executed in case of answer migrations.
3. The new template is copied into a temporary location. The possibly migrated answers from the real project are respected as "last answers" and otherwise regular user interaction is possible unless --data/--defaults flags are passed accordingly, thus, changes in the questionnaire can be addressed by explicit new answers.
4. A diff is created between the output of 1. and the real project.
5. The new template is copied into the real destination with the answers given in 3. and without any user interaction because those answers match the template version.
6. The diff from 3. is applied to the real project.
7. Files that exist in 1. but not in 2. are removed from the real project.
8. Post-migration tasks are executed on the real project.

---

This workflow seems to work according to the test suite, but the (modified) implementation in main.py::Worker._apply_update(...) is a bit hacky and might need some refactoring. For instance, blocks like

# Clear last answers cache to load possible answers migration
with suppress(AttributeError):
    del self.answers
with suppress(AttributeError):
    del self.subproject.last_answers

or

# Copy the new template output into the actual destination with the
# answers from the temporary destination.
# TODO: Do this more elegantly.
old_data = self.data
old_defaults = self.defaults
old_quiet = self.quiet
self.data = new_worker.answers.combined
self.defaults = True
self.quiet = True
try:
    self.run_copy()
finally:
    self.data = old_data
    self.defaults = old_defaults
    self.quiet = old_quiet

don't feel right, but I think some more fundamental refactoring is needed to improve them. I'd postpone it to a follow-up PR to keep the scope of this PR small and ship improvements more quickly.

Fixes #686. Related to #619.

/cc @pawamoy

[codecov]

Codecov Report

Merging #982 (fbbff61) into master (8987339) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #982      +/-   ##
==========================================
- Coverage   96.41%   96.27%   -0.15%     
==========================================
  Files          43       43              
  Lines        3317     3352      +35     
==========================================
+ Hits         3198     3227      +29     
- Misses        119      125       +6     

Flag	Coverage Δ
unittests	96.27% <100.00%> (-0.15%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tests/test_conditional_file_name.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[pawamoy]

Hello @sisp, thanks for the great work you're doing on this project 🚀 !

Your changes sound good to me. Would you also update the Mermaid diagram in the docs?

[sisp]

Thanks for the praise, @pawamoy! 🙏

I've updated the diagram and added a step index (e.g. (1), (2), ...) because the order is important and not obvious otherwise.

While changing the diagram, I decided to move step 4 (computing the diff) up and perform it after step 1 because it's clearer that computing the diff doesn't depend on answer migration. Also, the diagram becomes easier to read because there are fewer jumps when following the order of the steps. This change is a no-op.

[pawamoy]

Just checked the diagram, nice! Copier has a complex update process but with a bit of focus the diagram is perfectly readable 👍

[sisp]

I just resolved a merge conflict with master and added a missing type hint in the tests.

[yajo]

Sorry I had to do an important fix in #1068. Could you solve conflicts again please?

[yajo]

I fixed another problem in #1072 with an approach similar to yours. That does include a test that was failing. Could you check after rebasing on top of that (once it gets merged)?

[sisp]

That's great! I spent quite some time verifying that the recent fixes also fix this issue as a side effect. I suspected some missing edge case when answering interactively during the update process and extended the test case accordingly, but it looks like everything works fine now. Also, to the best of my knowledge the workflow diagram does not need to be changed anymore because there don't seem to be any changes in the workflow at the abstraction level of the diagram. So I reverted that change.

As it turns out, this PR now only adds a test case. You'll need to adjust the commit message accordingly. I followed your example in #1072 and parametrized the test case to cover both interactive and non-interactive answering instead of creating a second test case for interactive answering in tests/test_prompt.py.

[yajo]

As an exception I kept the fix prefix in the commit to let it appear in the next changelog. After all, it got fixed in the release. Thanks!