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

Improved logic for when for handling initial questions #1574

Open
stereobutter opened this issue Apr 5, 2024 · 14 comments
Open

Improved logic for when for handling initial questions #1574

stereobutter opened this issue Apr 5, 2024 · 14 comments
Labels
enhancement triage Trying to make sure if this is valid or not

Comments

@stereobutter
Copy link
Contributor

stereobutter commented Apr 5, 2024

Disclaimer This feature request/proposal is a spin-off from #1459 because I felt that that issue didn't get the real, underlying problem across.

Use Case

When setting up a new project via copier copy I'd like to be able and prompt the user for some initial information and then skip that question on any following copier update. This is both useful for answers that cannot be changed by updating a template (requiring the use of copier recopy) or rendering conditional files when the project is setup for the first time and not on updating.

The Problem with the current behaviour ofwhen

Take as en example the following question:

# copier.yaml
id:
  type: str
  help: "Enter an ID for your project"
  when: "{{id is not defined}}" 

when the project is first created by running copier copy the variable id is not defined and the user will be asked to enter an id and the answer is recorded

# .copier-answers.yml
_commit: <some commit hash>
id: <some-id-the-user-entered>
...

when the project is then later updated via copier update the question is skipped - nice, just what we wanted. However when we look at the recorded answers we'll notice that the previous answer is now gone

# .copier-answers.yml
_commit: <some other commit hash>

That is because (as per documentation) "If a question is skipped, its answer is not recorded [...]" which has to be read quite literally and is rather subtle (and I'd argue not what most people would read into that statement.)

If we then run copier update again we'll get ValueError("Question "id" is required"). The error is not really the point though (to be honest I have been looking at the code that raises this error for more than 15 minutes but I'm still not sure why it does what it does).

The point I'd argue is that the current behaviour is both a foot gun and impedes legitimate use cases that could be handled with when (like the example above). I could not think of any use case where the current behaviour is what users would want to happen.

Proposal

Change the implementation of when so it only hides the answer from the answer file if the answer is new (and thus does not remove a previous answer like seen above).

Questions

  • do other people feel this would be an improvement to copier and using when in this way should be a supported use case?
  • would this change be considered backwards compatible (ignoring https://xkcd.com/1172/)?
  • Is the current documentation of when's behaviour still correct with that change i.e. would people read "If a question is skipped, its answer is not recorded [...]" and expect this behaviour or do we need to be more specific here? I'd argue again that most people do not read the docs this literally and the current behaviour is actually more unexpected than the proposed behaviour.
@stereobutter
Copy link
Contributor Author

stereobutter commented Apr 5, 2024

@sisp
Copy link
Member

sisp commented Apr 6, 2024

I'm not able to see the difference between this issue and #1459. There, @yajo argued against freezing answers. How is your use case different?

That said, I myself have been struggling a little with freezing the first copyright year (when copier copy was first run, i.e. when the project was created). AFAIK, a copyright notice for software, which is continuously updated, should provide a year range:

Copyright (c) <year of first publication>-<year of most recent publication> <copyright holder>

For this, freezing <year of first publication> would be useful and <year of most recent publication> could be determined using, e.g., the jinja-time extension, so upon every copier update (independent of whether a new template release exists) the current year would be rendered.

If we were to consider this use case despite #1459 (comment), I'm a bit concerned that switching when: <falsy> behavior based on whether an answer has been recorded before may be too subtle/implicit.

@pawamoy
Copy link
Contributor

pawamoy commented Apr 6, 2024

Isn't there a way to use default to maintain the first recorded answer? 🤔

id:
  type: str
  help: "Enter an ID for your project"
  when: "{{id is not defined}}" 
  default: "{{ id if id is defined else None }}"

@sisp
Copy link
Member

sisp commented Apr 6, 2024

@pawamoy I don't see how this can work at the moment because whenever when: <falsy> then the answer isn't recorded, so it will be lost for the next update.

@stereobutter
Copy link
Contributor Author

stereobutter commented Apr 6, 2024

As I wrote at the beginning of my post, I felt that #1459 didn't manage to get to the actual issue all that well. It only mentions the current, rather subtle behaviour of when removing previous answers in passing. Also it proposes a new answer_once setting for questions, that in my opinion, would introduce unnecessary complexity and interact badly with existing copier features like when. In addition it includes a UI/UX request for somehow displaying these answer_once questions to the user. My proposal doesn't introduce any new primitives, no UI/UX features and implementation wise, I believe, is just a small change.

@sisp recording the date/year of project creation for license notices is another good example for these types of initial questions that do not get updated by copier update

@sisp
Copy link
Member

sisp commented Apr 14, 2024

Since you went ahead and created #1582, I gave your idea some more thought and don't think this approach is good.

Consider the following example:

  • copier.yml:

    _subdirectory: template/
    
    package_manager:
      type: str
      choices:
        - pip
        - poetry
        - pdm
    
    keep_lockfile:
      type: bool
      when: "{{ package_manager in ['poetry', 'pdm'] }}"
  • template/{{ _copier_conf.answers_file }}.jinja:

    # Changes here will be overwritten by Copier; NEVER EDIT MANUALLY
    {{ _copier_answers|to_nice_yaml -}}
  • template/.gitignore.jinja:

    {%- if not keep_lockfile %}
    {{ package_manager }}.lock
    {%- endif %}
  • template/renovate.json5.jinja:

    {
      // ...
    
      {%- if keep_lockfile %}
    
      "lockFileMaintenance": {
        "enabled": true,
        "schedule": "every 4 week between the 1 and 49 on Monday"
      },
      {%- endif %}
    
      // ...
    }

In this example template excerpt, a Python package manager is selected and – if it supports a lock file – one can choose whether the lock file should be Git-tracked and updated using Renovate. When you run

$ copier copy $src $dst
🎤 package_manager
   poetry
🎤 keep_lockfile (bool)
   Yes

then .gitignore contains poetry.lock and renovate.json5 contains the lock file maintenance config block. But when you later update your project to use pip

$ copier update $dst
🎤 package_manager
   pip

then the keep_lockfile question isn't asked because of when: "{{ package_manager in ['poetry', 'pdm'] }}" (so far so good), but the only change in the project will be:

 _commit: <tag>
 _src_path: <template_url>
 keep_lockfile: true
-package_manager: poetry
+package_manager: pip

Notice that the .copier-answers.yml file still contains the keep_lockfile: true record, which shouldn't exist anymore since pip supports no lock file, and there is no change in .gitignore and renovate.json5 because the Jinja context still has the variable keep_lockfile with value True although it shouldn't be defined at all.

This example shows that your idea won't allow a conditional question without a default value to change from when: true to when: false, which may lead to incorrect rendering results.

@stereobutter
Copy link
Contributor Author

stereobutter commented Apr 14, 2024

@sisp I think you can solve the issue by using a separate "computed value" for deciding whether to render the lockfile or not.

package_manager:
  type: str
  choices:
    - pip
    - poetry
    - pdm

keep_lockfile: 
  type: bool
  help: Do you want to create a lockfile?
  when: "{{ package_manager in ['poetry', 'pdm'] }}"

render_lockfile:  # this computed value is used to determine whether to render a lockfile
  type: bool
  default: "{{ keep_lockfile and package_manager in ['poetry', 'pdm'] }}"
  when: false

This way

  • keep_lockfile is only asked when the selected backend supports lockfiles
  • render_lockfile is true when the backend supports lockfiles and keep_lockfile is also true
  • when running copier update and selecting pip the lockfile is deleted
  • when running copier update again and selecting pdm or poetry the user is again prompted for keep_lockfile

@sisp
Copy link
Member

sisp commented Apr 14, 2024

That would be a workaround, but your idea would be a breaking change. I'm not convinced by the behavior that switching from when: true to when: false keeps the previously recorded answer. WDYT, @copier-org/maintainers?

@pawamoy
Copy link
Contributor

pawamoy commented Apr 14, 2024

I'll need a bit of time to re-read and understand everything 🙂

@stereobutter
Copy link
Contributor Author

If it helps I think a good mental model for the proposed behaviour of when is "do you want to answer or update this question?" Using that mental model it makes sense why this doesn't work

keep_lockfile:
  type: bool
  when: "{{ package_manager in ['poetry', 'pdm'] }}"

and instead you'd need

keep_lockfile: 
  type: bool
  help: Do you want to create a lockfile?
  when: "{{ package_manager in ['poetry', 'pdm'] }}"

render_lockfile:  # this computed value is used to determine whether to render a lockfile
  type: bool
  default: "{{ keep_lockfile and package_manager in ['poetry', 'pdm'] }}"
  when: false

@stereobutter
Copy link
Contributor Author

I've though a bit more about this and I've come to share @sisp concerns regarding backwards compatibility and I've tried to come up with a different design. What do you think about

year_of_first_publication:
  type: str
  when: "{{year is not defined}}"
  retain: true 

where retain controls whether the previous answer is kept if when evaluates to false. For backwards compatibility retain would default to false.

@yajo
Copy link
Member

yajo commented Apr 16, 2024

Hello and thanks for your proposals!

I think the use case fits better into a simple pre-commit hook or unit test that makes sure certain values in the answers file don't change.

You can record the initial values that should be frozen in a separate file, only the 1st time. Then add it to _skip_if_exists so it stays untouched on updates. Finally, assert that the copier answers file contains those frozen answers, among others.

Have you considered that?

@yajo yajo added the triage Trying to make sure if this is valid or not label Apr 16, 2024
@stereobutter
Copy link
Contributor Author

stereobutter commented Apr 21, 2024

Similar to the feature requests (e.g. #240) about being able to run tasks selectively depending on whether the project is being copied or updated, I believe asking a question just on running copier copy and recording the answer would be a useful feature. It's not just about preserving an answer here, it's also about UX for the user of my template i.e. not asking a "setup" question again.

In another (recent) case I wanted to create an initial secret on project creation that would be rendered into a file (that's on .gitignore so skip_if_exists won't work here).

I've tinkered a bit more with this and I think I found a workaround that I'd like to share with you. Take for example

# copier.yaml

run_setup:
  # computed value to determine whether to prompt the setup questions
  type: bool
  default: "{{recorded_setup is not defined or recorded_setup is false}}"
  when: false

username:
  type: str
  when: "{{run_setup}}"

password:
  type: str
  help: "initial password"
  secret: true
  default: ""
  when: "{{run_setup}}"

user_provided_year_of_creation:
  type: str
  help: When was the project created?
  when: "{{run_setup}}"

year_of_creation:
  type: str
  # computed value for the year of creation from
  # either the previously recorded answer
  # or the answer the user just provided during setup
  default: "{{recorded_year_of_creation or user_provided_year_of_creation}}"
  when: false

message:
  # just a regular question for comparision
  type: str

with this modified {{_copier_conf.answers_file}}.jinja

# {{_copier_conf.answers_file}}.jinja
# Changes here will be overwritten by Copier
{{ _copier_answers|to_nice_yaml -}}
recorded_setup: true
recorded_year_of_creation: "{{year_of_creation}}"

Using this trick for recording additional data in the answers file and running copier copy the questionnaire the user is presented is

🎤 username
   Sascha
🕵️ initial password
   ******
🎤 When was the project created?
   2024
🎤 message
   hello world

On any following copier update the setup questions (name, password and user_provided_year_of_creation ) are skipped while the year_of_creation and run_setup variables are still available for rendering templates.

🎤 message
   hello copier

Awesome, just what we wanted right?

Yes where copier update is concerned this works just as intended (apart from some minor diffs in the answers file between the first and any subsequent runs of copier update due to user_provided_year_of_creation and username being recorded the first time around and then deleted by the falsy when condition.)

What doesn't work at the moment (and what I'd consider a bug in copier) is running copier copy over the existing project again when the user would like to rerun the setup phase. Despite a comment in the code for run_copy saying

Generate a subproject from zero, ignoring what was in the folder

I found out that Subproject still locates and injects the data from the copier answers file in this case. If you also consider this to be a bug fixing this would fully satisfies my use cases given the workaround above.

Proposal

  • fix copier copy so that it ignores the answers file when copying over an existing project
  • bless the workaround above as a solution to these use cases by including an example in the docs (if you deem this to be a low risk for future breakage and/or potential conflict with future development)

@yajo
Copy link
Member

yajo commented Apr 29, 2024

Hah, very smart! ❤️

  • fix copier copy so that it ignores the answers file when copying over an existing project

I usually take benefit of that feature when, for example, an upstream template lacks some patches. Then I recopy using copier copy $my_fork ./preexisting_project and let Copier update from my fork from there until upstream merges what I need.

So, it'd be not so nice if we lost that feature.

However, in the case of your template, you can ask your users to do copier [copy|update] -d recorded_setup=false if they want to re-run the setup phase.

  • bless the workaround above as a solution to these use cases by including an example in the docs (if you deem this to be a low risk for future breakage and/or potential conflict with future development)

Blessed you are 🤴🏼

Just please push a PR with docs about this. Probably under the FAQ section. That'll be the final bliss. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement triage Trying to make sure if this is valid or not
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants