Handle default body text when creating issues and pull requests#3658
Handle default body text when creating issues and pull requests#3658
Conversation
There was a problem hiding this comment.
I wasn't able to fully trace why the commit messages were erased when skipping during
gh pr create. However, my best guess is that, because the editor is never opened, the default text is never written to a temporary file. Because nothing was written, the survey returns nothing and overwrites the commit messages instate.Bodywith an empty string.
That sounds about right! Thanks so much for taking a deep dive into this.
I do, however, feel somewhat strongly about fixing this without changing the actual Survey prompt around the editor for now. The prompt could definitely use some redesign, but I would want that redesign to be motivated by enhancing usability instead of trying to fix a bug. In other words, I would like to see the change to rethink the prompt be separate from the change to fix this bug, and when we do change the prompt, we would have to consider whether it's backward-compatible with users for whom the old prompt (press e to open editor) is already part of muscle memory.
Do you think it's possible to keep using GHEditor and just handle the case of preserving the body if the editor wasn't invoked?
|
I think this fix is done. The one additional thing I did was convert the list of if statements for detecting keyboard presses to a switch statement (commit 6db87ff). |
chemotaxis
left a comment
There was a problem hiding this comment.
I reverted the initial changes and have a solution at commit 6db87ff.
|
Hi @mislav! I'm just checking to see if there is something else I need to fix in this pull request. It looks like |
If we are allowed to skip the editor _and_ we want to append the default text to the editor if we'd opened it, we just return the default text. Co-Authored-By: Mislav Marohnić <mislav@github.com>
There was a problem hiding this comment.
Thank you for your patience! The change looks good, but there was a lot of renaming going on, and even though I agree with your motivations for renaming, right now I wanted us to just focus on making the smallest change possible to fix the bug. I've condensed the fix even further and added a test.
|
Understood. Thanks for adding the tests and for merging! Do those renamings or refactorings manifest as separate issues that I can contribute to or do those kinds of issues get taken care of periodically by the core team? |
|
@chemotaxis We periodically address technical debt by refactoring and shuffling code around, of course! We are open to receiving those kinds of contributions as well, but keep in mind that we expect that a refactoring yields some tangible improvement (e.g. that the new code is slimmer, or easier to test, or more portable, or more resilient against edge cases) and that the author is mindful of potential regressions (e.g. avoids changes to code that has little test coverage). Otherwise, if a change is purely cosmetic, such as renaming a struct field (even if the new name is slightly more descriptive), we are less keen on accepting those since the risk-to-benefit ratio isn't in our favor. |
This comment refers to the original description: #3658 (review)
This fixes #674.
After a second look at the code,
surveyext.GhEditorexplicitly returns an empty string when pressingEnter. It doesn't look like it had anything to do with unwritten temporary files. I added an extra if clause to see if the editor prompt was instructed to add the default value to the editor.Unfortunately, adding tests for this isn't straightforward and, so far, no tests exist for
survey.GhEditor. If you still want me to try to add tests, let me know.The
GhEditoris used in these cases:gh issue creategh issue editgh pr creategh pr editgh pr reviewgh issue createandgh pr createare the only places where the new condition will trigger. The original behavior (returning an empty string when skipping) seem to be preserved everywhere else.