Skip to content
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

feat: Adding '--skip-answered' flag to enhance user interaction during copier update #1276

Merged

Conversation

ljossha
Copy link
Contributor

@ljossha ljossha commented Jul 28, 2023

This Pull Request addresses a usability issue troubling users when they run 'copier update'. Previously, each time 'copier update' was run, the user had to review all responses given in the answers file. This was a repetitive and laborious process, particularly for users dealing with extensive templates.

This PR introduces a new flag- --skip-answered to improve the user experience and reduce friction during updates. The new flag allows users to bypass the review of questions already answered in the past. It automatically pulls responses from the answers file and presents only the newly added template questions to the user for their review.

skip-answered will skip all answered questions and ask user input only for new inputs if they are present.

Issue: #1178

@ljossha ljossha changed the title Adding '--skip-answered' flag to enhance user interaction during copier update feat: Adding '--skip-answered' flag to enhance user interaction during copier update Jul 28, 2023
@yajo yajo linked an issue Jul 30, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jul 30, 2023

Codecov Report

Merging #1276 (8cdc16d) into master (a9e7b6b) will increase coverage by 0.16%.
Report is 25 commits behind head on master.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
+ Coverage   96.95%   97.12%   +0.16%     
==========================================
  Files          47       48       +1     
  Lines        3969     4238     +269     
==========================================
+ Hits         3848     4116     +268     
- Misses        121      122       +1     
Flag Coverage Δ
unittests 97.12% <97.77%> (+0.16%) ⬆️

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

Files Changed Coverage Δ
copier/main.py 95.99% <50.00%> (-0.09%) ⬇️
copier/cli.py 100.00% <100.00%> (ø)
tests/test_cli.py 100.00% <100.00%> (ø)
tests/test_prompt.py 93.87% <100.00%> (+2.44%) ⬆️

... and 8 files with indirect coverage changes

@yajo yajo self-assigned this Jul 30, 2023
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

The code looks perfect to me. Just a minor non-blocking comment. Thanks for this contribution!

It seems you still have a little style issue... if you can run pre-commit it'll fix it for you.

copier/main.py Outdated Show resolved Hide resolved
Co-authored-by: Jairo Llopis <[email protected]>
@ljossha
Copy link
Contributor Author

ljossha commented Jul 30, 2023

The code looks perfect to me. Just a minor non-blocking comment. Thanks for this contribution!

It seems you still have a little style issue... if you can run pre-commit it'll fix it for you.

I've fixed your suggested changes.
But when I try to run pre-commit, I receive:

An error has occurred: InvalidConfigError: 
=====> .pre-commit-config.yaml is not a file

Any ideas?

And another question: how long will it take to deploy in the brew after merging my changes?

UPD:
On running poe lint it returns:
image

@yajo
Copy link
Member

yajo commented Jul 31, 2023

You will have to run poe lint (as you did), and commit and push the changes.

Next release will still take some weeks. I don't want to release while on holidays. 🏖

@ljossha
Copy link
Contributor Author

ljossha commented Jul 31, 2023

You will have to run poe lint (as you did), and commit and push the changes.

Next release will still take some weeks. I don't want to release while on holidays. 🏖

Yeah, I see. Pushed new changes.

@ljossha
Copy link
Contributor Author

ljossha commented Aug 3, 2023

@yajo Hey man!
It seems that everything is fine.
Where will you be able to merge it?
Do you need anything from me at this point?

@yajo
Copy link
Member

yajo commented Aug 3, 2023

Since this is a new feature, I'd want to hand-test it before merge, so this will take a bit more than usual due to holidays. Maybe 1 or 2 weeks. But for now you can relax. According to the code, it seems perfect, so I'll just merge when I can give it a try. Otherwise I'll post what to change. Thanks!

tests/test_migrations.py Outdated Show resolved Hide resolved
@yajo yajo enabled auto-merge (squash) September 3, 2023 21:06
@yajo yajo merged commit 8619fc8 into copier-org:master Sep 3, 2023
17 checks passed
@ljossha ljossha deleted the dont-require-users-to-review-all-questions branch September 3, 2023 21:47
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.

Don't require users to review all questions when running copier update
2 participants