Disable preview option in prompts if URL size is too long#3271
Disable preview option in prompts if URL size is too long#3271
Conversation
|
Is there a way to test that a specific option hasn't been rendered by Also, I'm not sure how should |
|
@cristiand391 As far as I know we don't have a good way to automatically test that the option is not being rendered by For a first iteration I am okay with not displaying any warning that the preview option has been disabled because I expect it is a fairly rare case that the url will be greater than 8192 bytes but @mislav might have another opinion. Overall this code looks solid to me. Thanks for the contribution. |
mislav
left a comment
There was a problem hiding this comment.
Thanks for working on this!
To let users know why the "Continue in browser" option is not presented to them, we could print a line to stderr just before calling ConfirmSubmission(). This isn't strictly necessary, but if someone is used to choosing "Continue in browser", they might find it weird that the option is mysteriously not present at some times but present at other times.
Is there a way to test that a specific option hasn't been rendered by
survey? I've only added tests for--webflag + long URL case.
No, I don't think this is easy to do right now, so you are free to just skip that and test it manually. 👌
pkg/cmd/issue/create/create.go
Outdated
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
It doesn't look like this condition is needed
pkg/cmd/issue/create/create.go
Outdated
| return | ||
| } | ||
| if !utils.ValidURL(openURL) { | ||
| return fmt.Errorf("Failed to create URL: maximum URL length exceeded") |
There was a problem hiding this comment.
Due to our failure recovery mechanism in issue/pr create, new errors should first be assigned to err before they are returned.
| return fmt.Errorf("Failed to create URL: maximum URL length exceeded") | |
| err = fmt.Errorf("Failed to create URL: maximum URL length exceeded") | |
| return |
There was a problem hiding this comment.
Do you think gh should offer recover instructions for this error?
I tried assigning errors to err but it in order to make it work I had to move the defer statement preserveInput right before this block.
a0b9cb6 to
57536e7
Compare
mislav
left a comment
There was a problem hiding this comment.
Fantastic. Thank you so much for the hard work!
The GitHub GET endpoint for opening a new issue has a restriction for maximum length of a URL: 8192 bytes cli/cli#3271 cli/cli#1575 https://github.com/cli/cli/blob/trunk/pkg/cmd/issue/create/create.go#L167 https://github.com/cli/cli/blob/trunk/utils/utils.go#L84
The GitHub GET endpoint for opening a new issue has a restriction for maximum length of a URL: 8192 bytes cli/cli#3271 cli/cli#1575 https://github.com/cli/cli/blob/trunk/pkg/cmd/issue/create/create.go#L167 https://github.com/cli/cli/blob/trunk/utils/utils.go#L84
Fixes #1575
This PR adds an additional check to
pr/issue createto disableContinue in browseroption if the preview URL max size (8192 bytes) is exceeded.