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: add support for pre-copy and post-copy messages #1194

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

sisp
Copy link
Member

@sisp sisp commented Jun 15, 2023

I've added support for pre-copy and post-copy messages that can be written in plain text or markdown. Messages can be written inline or included via Jinja includes, and messages are rendered.

I'll work on pre-update and post-update messages in a separate PR once we've clarified whether the implementation in this PR is good.

Closes #723.

@sisp sisp force-pushed the feat/copy-messages branch 2 times, most recently from 528d1cb to 06194a7 Compare June 15, 2023 12:34
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #1194 (8841a28) into master (32f2a3a) will increase coverage by 0.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
+ Coverage   96.74%   97.17%   +0.43%     
==========================================
  Files          47       47              
  Lines        3936     3969      +33     
==========================================
+ Hits         3808     3857      +49     
+ Misses        128      112      -16     
Flag Coverage Δ
unittests 97.17% <100.00%> (+0.43%) ⬆️

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

Impacted Files Coverage Δ
copier/main.py 96.07% <100.00%> (+0.42%) ⬆️
copier/template.py 96.34% <100.00%> (-0.02%) ⬇️
tests/test_output.py 100.00% <100.00%> (ø)

... and 19 files with indirect coverage changes

Copy link
Contributor

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Nice!

If we print these messages through Rich, I think we should do it for questions description/help too, for consistency. Users might be tempted to use Markdown in questions description/help and would be surprised to see that it does not get rendered the same way as pre/post messages. This can of course be done in another PR.

@sisp
Copy link
Member Author

sisp commented Jun 15, 2023

Is that possible? The help text is printed through Questionary. It would be great though!

@pawamoy
Copy link
Contributor

pawamoy commented Jun 15, 2023

I don't know 😄 Maybe it's possible but hard and not worth it. I'd be fine with Markdown rendering in just pre/post messages 🙂

@sisp
Copy link
Member Author

sisp commented Jun 15, 2023

I'll check it out.

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.

There's something wrong here. I've tested the test interactively and this is what happens:

➤ copier copy '/tmp/pytest-of-yajo/pytest-0/popen-gw0/src0' /tmp/b
🎤 project_name
   a
Thank you for using our template                                                                                                                                                                           

 • You're on linux                                                                                                                                                                                         
 • Let's generate your project                                                                                                                                                                             

Copying from template version None
    create  .

Project a successfully created                                                                                                                                                                             

Next steps:                                                                                                                                                                                                

 • cd /tmp/b                                                                                                                                                                                               
 • Start coding                                                                                                                                                                                            

The welcome message appears after the questionary. It should be before, right?

@yajo
Copy link
Member

yajo commented Jun 15, 2023

If we print these messages through Rich, I think we should do it for questions description/help too, for consistency.

Yikes, yet another method of printing stuff... 🤦🏼‍♂️ #368 gets bigger.

@sisp
Copy link
Member Author

sisp commented Jun 15, 2023

Oh, good catch about the printing after the question. It should be before the question, of course. Strange, I'll look into it. It seems I'll need to change the tests to use a tui, was hoping to avoid that.

@sisp
Copy link
Member Author

sisp commented Jun 15, 2023

If we print these messages through Rich, I think we should do it for questions description/help too, for consistency.

Yikes, yet another method of printing stuff... 🤦🏼‍♂️ #368 gets bigger.

We should work on consolidating all those printing methods as much as possible.

copier/main.py Outdated Show resolved Hide resolved
@sisp
Copy link
Member Author

sisp commented Jun 29, 2023

I've extended the tests to also check interactive prompting and make sure the message before copy is shown before the prompt (at the beginning of the log that also contains the prompt in interactive mode) and the message after copy is shown after the prompt (at the end of the log that also contains the prompt in interactive mode).

I'd appreciate another critical review. 🙏

@sisp sisp requested review from pawamoy and yajo June 29, 2023 14:27
copier/main.py Outdated Show resolved Hide resolved
@lhupfeldt
Copy link

I think this could be made more general by replacing the message_before_copy with just message and allowing messages mixed with questions (and vars when we get when: false back). Of cause a message may only refer context already resolved. That way grouping of questions with message will be possible.
Is message_after_copy only for the copy operation or will be printed after all operations? I think the default should be always, and if necessary a when: operation=update/copy/... could be added (later). In that case a more generic name, like post_message would be better.

@sisp
Copy link
Member Author

sisp commented Jul 8, 2023

I'm afraid I don't understand your proposal. Could you provide an example of how you think copier.yml would look?

@lhupfeldt
Copy link

I'm afraid I don't understand your proposal. Could you provide an example of how you think copier.yml would look?

Something like:

_tasks:
  - exec: "./doit.sh"
    message: "We did it!"

_message: "Overall template intro."

_message: "Your Personal info"

name: "Your full name"
email: "Your email"

_message: "Family info"

mother: "Your mother's name"
father: "Your father's name"

_message:
  text: "Ouch, your name is 'dummy'?"
  when: "{{ name == 'dummy' }}"

_message:
  text: "Before tasks first task"
  when: has_tasks

# Here we run tasks, so there may be task message output, or output printed from tasks.
# A `message` method could also be made available to take advantage of standardized formatting of messages printed from task.

_message:
  text: "Yeah"
  when: "{{ copier_finished and copier_success copier_action == 'update' }}"

_message:
  text: "Congratulation"
  when: "{{ copier_finished and copier_success }}"

_message:
  text: "Oops!"
  when: "{{ copier_finished and copier_error }}"

I think that a single _message with a when condition is both more flexible and more user friendly.
The example is just some ideas, don't take it literally.
Task messages are just an extra feature.

@sisp
Copy link
Member Author

sisp commented Jul 8, 2023

This isn't valid YAML according to the spec (https://yaml.org/spec/1.2.2/#mapping):

The content of a mapping node is an unordered set of key/value node pairs, with the restriction that each of the keys is unique.

PyYAML overwrites the result with the last key.

@lhupfeldt
Copy link

This isn't valid YAML according to the spec (https://yaml.org/spec/1.2.2/#mapping):

You are right.
I guess the only way to make a generic messaging mechanism similar to my suggestion would be to handle _message_X where X can be anything.

@sisp sisp force-pushed the feat/copy-messages branch 2 times, most recently from 61bbb27 to 62d2768 Compare July 10, 2023 18:46
@sisp sisp requested a review from yajo July 11, 2023 09:33
@yajo
Copy link
Member

yajo commented Jul 19, 2023

Apart from being invalid yaml, you already have the question help text to print whatever you want, so that proposal doesn't add much value IMHO.

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.

Really good, thanks!

Just one minor suggestion for the docs part. Non blocking.

docs/configuring.md Outdated Show resolved Hide resolved
@lhupfeldt
Copy link

lhupfeldt commented Jul 19, 2023 via email

@sisp
Copy link
Member Author

sisp commented Jul 19, 2023

@lhupfeldt Only before copy, not before update. But you got me thinking and it turns out this message is printed also for recopy. I'll extend the docs and add some tests.

@sisp
Copy link
Member Author

sisp commented Jul 19, 2023

I've extended the tests to check the message printing also for recopying and edited the docs accordingly.

Since those changes don't alter the PR in general, I'll take the liberty of merging now without further review. I hope that's okay, @copier-org/maintainers.

@sisp sisp merged commit dc94486 into copier-org:master Jul 19, 2023
17 checks passed
@sisp sisp deleted the feat/copy-messages branch July 19, 2023 11:13
@lhupfeldt
Copy link

lhupfeldt commented Jul 19, 2023 via email

@sisp
Copy link
Member Author

sisp commented Jul 20, 2023

To avoid repetition, you could rely on YAML anchors and aliases to print the same message before copy and update (assuming _message_before_update was already available):

_message_before_copy: &message_before Hello, thanks for using our template!
_message_before_update: *message_before

This way, it's explicit when the message will be printed.

@lhupfeldt
Copy link

lhupfeldt commented Jul 20, 2023 via email

@yajo
Copy link
Member

yajo commented Jul 21, 2023 via email

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.

Add intro message
4 participants