verdi computer/code duplicate now reuses prepend_text and append_text #3788
Conversation
This required adding a new ContextualDefaultOption class to get a default callable that also receives the context. Moreover, a but was fixed in the function that prompts for the prepend/append_text, as if the file was not touched/modified, instead of reusing the text provided in input, an empty text was used instead.
|
@ConradJohnston could you check that this work as you expect? If this is the case, after adding tests this is ready to merge |
There was a problem hiding this comment.
Works great, thank you.
One curiosity - why does duplicate allow me to have identical labels for the code?
If one uses orm.Code.get_from_string(), it complains unless the code@computer label is unique.
I understand that this isn't the ID on which uniqueness is enforced, but given that there are convenience functions which do use this label, should we warn the user? Will make a separate issue if you agree that it's an oddity.
We should, and there already is an issue for it #3303 |
|
Thanks @ConradJohnston ! Do you happen to have time to add a test, by chance? I'll be a bit busy in the next days and if we have tests we can merge this. |
|
@giovannipizzi , yes, I'll try to get looking at it as soon as possible. |
Sets prepend/append text when setting up the computer/code for the non-interactive tests. The correct duplication was already tested by an assertion, but the prepend and append texts were set to the default value of an empty string.
|
@giovannipizzi , have test coverage now. Can I push back to your branch? |
|
@ConradJohnston sure, thanks a lot! I just added you as a collaborator to my fork - not sure if this means you can push, in case you cannot I can set this after you accept the invite (GitHub might have changed this workflow recently? I remember I could set the role while inviting...) |
Hi Giovanni , I've pushed my change now, so we'll need another reviewer. |
|
Thanks a lot @ConradJohnston! I cannot approve this myself - either you reprove it, or maybe @sphuber could approve and merge? |
This required adding a new ContextualDefaultOption class
to get a default callable that also receives the context.
Moreover, a but was fixed in the function that prompts
for the prepend/append_text, as if the file was not touched/modified,
instead of reusing the text provided in input, an empty text was used instead.
Fixes #3747