Skip to content

Explicitly replace empty body with default template#497

Merged
mislav merged 1 commit into
cli:masterfrom
ShahzadUmair:fix-template
Feb 19, 2020
Merged

Explicitly replace empty body with default template#497
mislav merged 1 commit into
cli:masterfrom
ShahzadUmair:fix-template

Conversation

@ShahzadUmair

@ShahzadUmair ShahzadUmair commented Feb 18, 2020

Copy link
Copy Markdown
Contributor

This pr fixes 388.

Survey editor returns an empty string if we skip the prompt editor.go:98. In this Pr we explicitly set the body equal to template if body is empty and the template content is available.

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you! The fix looks good. I wish I could advise you to add an automated test for this, but alas we don't yet have tests for the interactive portions of issue/pr create flows.

I'm wondering whether we should even proceed in this case? In my view as an open source maintainer, if a person hadn't edited the template at all, perhaps we shouldn't let it submit in the first place until they edit it.

On the other hand, maybe such change of functionality should be left for another PR.

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After a short deliberation, I've decided that the fix is good for now and that we should discuss a potential change in fuctionality separately. Thank you! 🌟

@mislav mislav merged commit f6c9e7b into cli:master Feb 19, 2020
@billygriffin

Copy link
Copy Markdown
Contributor

Maybe a dumb question, but it’s not immediately apparent whether this also covers the issue create flow or just pr create. Would you mind clarifying that please, as @tierninho laid out the scenarios it doesn’t work as expected for issue creation as well.

@mislav

mislav commented Feb 19, 2020

Copy link
Copy Markdown
Contributor

@billygriffin Certainly! This logic is shared equally between issues and prs, so any change affects both.

tb, err := titleBodySurvey(cmd, title, body, templateFiles)

tb, err := titleBodySurvey(cmd, title, body, templateFiles)

@billygriffin

Copy link
Copy Markdown
Contributor

Wonderful, thank you!

@ShahzadUmair

Copy link
Copy Markdown
Contributor Author

Thank you! The fix looks good. I wish I could advise you to add an automated test for this, but alas we don't yet have tests for the interactive portions of issue/pr create flows.

Will look into how we can test the interactive parts and discuss the possibilities in a 1 pager (unless someone is already on it)? Or maybe will send a PR if I am feeling confident about the tests then ✨

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.

GHCLI Created PR's do not use Pull Request Template if the PR is directly submitted without viewing

3 participants