Implemented request-body data insertion through text-editor#45
Implemented request-body data insertion through text-editor#45athul merged 9 commits intohoppscotch:masterfrom
Conversation
There was a problem hiding this comment.
There are some idioms that were missed that are typically used in Go, a possibility for filesystem pollution with stray files, a possible runtime panic, some extra allocations (from redundant type conversion), and one usability concern.
The major thing is the runtime panic. 😄
gbmor
left a comment
gbmor
left a comment
There was a problem hiding this comment.
Realized I left this out of my initial review, an important error return value was discarded.
|
@devblin change that to |
I updated it within my last commit 👍🏽 . |
|
@gbmor I request another round of review 🙈 |
|
@devblin please add the required changes to the readme too ✨ |
gbmor
left a comment
gbmor
left a comment
There was a problem hiding this comment.
Looks pretty much good to me, as far as the code goes. I had one question about calling notepad which I don't know the answer to because I don't have a windows machine to test it on, and I've only ever written software targeting Linux/OpenBSD/FreeBSD, but I tagged it in the review. @athul maybe you know? Or have a windows machine to test it on?
Also, maybe add a blurb in the readme about using -e and how it checks $EDITOR then falls back to nano or notepad?
| if currentOs == "linux" || currentOs == "darwin" { | ||
| editor = "nano" | ||
| } else if currentOs == "windows" { | ||
| editor = "notepad" |
There was a problem hiding this comment.
I don't have a windows machine to test this, and I haven't really used windows since 2006-2007 so I'm just guessing here: but will this still find notepad without the file extension, or should it be notepad.exe?
There was a problem hiding this comment.
@gbmor I have tested it on windows machine too. And as exec.LookPath() function searches for executable files only, we can use both notepad and notepad.exe
There was a problem hiding this comment.
@devblin OK cool! Good to know. I'll remember that for when I have to target Windows in the future 👍
Updated README 👍🏽 |
athul
left a comment
athul
left a comment
There was a problem hiding this comment.
Just some small stuff I think would be good, you can accept or ignore it completely. Anyways looks good to me. Sorry for taking forever to merge this. Was a bit tied up with some stuff
|
I missed to mention that in Windows the print statement mentioned below wasn't properly color-encoding the Line 172 in 8e85342 Instead, as mentioned in https://github.com/fatih/color/blob/master/doc.go#L89 , we should use fmt.Fprintf(color.Output, out)
|
Okay. Haven't had the chance to test everything on windows(I don't have a windows machine either). It'd be great if you could push that too |
|
Done |
4 participants
Implements #41
For methods with request body, this update now allows user to insert request body data through editor.
For example:
$ hopp-cli post https://reqres.in/api/users/2 -c js -b '{"name": "morp","job": "zion resident"}'$ hopp-cli post https://reqres.in/api/users/2 -c js -e