Implemented request-body data insertion through text-editor #45

devblin · 2021-12-07T18:26:52Z

Implements #41

For methods with request body, this update now allows user to insert request body data through editor.
For example:

Currently, this is what we do to add request body:
$ hopp-cli post https://reqres.in/api/users/2 -c js -b '{"name": "morp","job": "zion resident"}'
Now, we can also use -e flag, indicating request data insertion through text-editor:
$ hopp-cli post https://reqres.in/api/users/2 -c js -e

gbmor

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. 😄

methods/basic.go

gbmor

Realized I left this out of my initial review, an important error return value was discarded.

methods/basic.go

athul

Hey @devblin the PR seems fine. Please address the changes and feedback given by @gbmor and me 🤝

methods/basic.go

devblin · 2021-12-08T08:45:13Z

@athul I have updated the changes, and I was getting some conflicts, I have resolved the conflicts in local with changes from master and I am getting error here:

hopp-cli/methods/basic.go

Line 29 in 8fc5452

body, err = io.ReadAll(os.Stdin)

as undefined: io.ReadAll, so should I updated it within this PR ?

gbmor · 2021-12-08T23:31:29Z

@devblin change that to ioutil.ReadAll(...) ... it's related to my other PR which was to bring hopp-cli up to Go1.16, and that function didn't exist before 1.16

devblin · 2021-12-09T04:09:57Z

@devblin change that to ioutil.ReadAll(...) ... it's related to my other PR which was to bring hopp-cli up to Go1.16, and that function didn't exist before 1.16

I updated it within my last commit 👍🏽 .

athul · 2021-12-15T15:43:50Z

@gbmor I request another round of review 🙈

athul · 2021-12-15T15:45:07Z

@devblin please add the required changes to the readme too ✨

gbmor

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?

gbmor · 2021-12-15T23:47:29Z

methods/basic.go

+			if currentOs == "linux" || currentOs == "darwin" {
+				editor = "nano"
+			} else if currentOs == "windows" {
+				editor = "notepad"


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?

@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

@devblin OK cool! Good to know. I'll remember that for when I have to target Windows in the future 👍

devblin · 2021-12-16T07:42:27Z

@devblin please add the required changes to the readme too ✨

Updated README 👍🏽

athul

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

README.md

devblin · 2021-12-18T06:40:00Z

I missed to mention that in Windows the print statement mentioned below wasn't properly color-encoding the out in terminal.

hopp-cli/cli.go

Line 172 in 8e85342

fmt.Println(out)

Instead, as mentioned in https://github.com/fatih/color/blob/master/doc.go#L89 , we should use
fmt.Fprintf(color.Output, out)

athul · 2021-12-19T04:13:38Z

I missed to mention that in Windows the print statement mentioned below wasn't properly color-encoding the out in terminal.

hopp-cli/cli.go

Line 172 in 8e85342

fmt.Println(out)

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

devblin · 2021-12-19T04:59:02Z

Done

athul · 2021-12-21T14:51:56Z

I guess this is ready to be merged. Thank you @devblin and @gbmor 🎉 🎉

devblin and others added 2 commits December 7, 2021 18:39

added feature to add request body through text-editor

65be171

resolved file busy issue

50f519c

devblin marked this pull request as draft December 7, 2021 18:27

gbmor reviewed Dec 7, 2021

View reviewed changes

gbmor reviewed Dec 8, 2021

View reviewed changes

methods/basic.go Outdated Show resolved Hide resolved

liyasthomas requested a review from athul December 8, 2021 00:50

athul suggested changes Dec 8, 2021

View reviewed changes

methods/basic.go Show resolved Hide resolved

methods/basic.go Outdated Show resolved Hide resolved

methods/basic.go Outdated Show resolved Hide resolved

devblin added 3 commits December 8, 2021 10:38

updated error verb & minor changes

a54db5d

resolved changes

cb50be1

minor refactor

72595d6

devblin marked this pull request as ready for review December 8, 2021 08:27

resolved conflicts

e0c30de

athul approved these changes Dec 9, 2021

View reviewed changes

devblin force-pushed the feat/editor branch from 3aeff23 to e0c30de Compare December 15, 2021 05:20

gbmor reviewed Dec 15, 2021

View reviewed changes

updated readme

42b8efd

devblin force-pushed the feat/editor branch from 3aeff23 to e0c30de Compare December 16, 2021 06:52

athul approved these changes Dec 17, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

updated readme with suggested changes

13ebe2c

updated print statement for formatted output

9da5a35

athul approved these changes Dec 21, 2021

View reviewed changes

athul merged commit f3d27cf into hoppscotch:master Dec 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented request-body data insertion through text-editor #45

Implemented request-body data insertion through text-editor #45

devblin commented Dec 7, 2021

gbmor left a comment •

edited

Loading

gbmor left a comment

athul left a comment

devblin commented Dec 8, 2021

gbmor commented Dec 8, 2021

devblin commented Dec 9, 2021

athul commented Dec 15, 2021

athul commented Dec 15, 2021

gbmor left a comment

gbmor Dec 15, 2021

devblin Dec 16, 2021 •

edited

Loading

gbmor Dec 17, 2021

devblin commented Dec 16, 2021

athul left a comment

devblin commented Dec 18, 2021

athul commented Dec 19, 2021

devblin commented Dec 19, 2021

athul commented Dec 21, 2021

Implemented request-body data insertion through text-editor #45

Implemented request-body data insertion through text-editor #45

Conversation

devblin commented Dec 7, 2021

gbmor left a comment • edited Loading

Choose a reason for hiding this comment

gbmor left a comment

Choose a reason for hiding this comment

athul left a comment

Choose a reason for hiding this comment

devblin commented Dec 8, 2021

gbmor commented Dec 8, 2021

devblin commented Dec 9, 2021

athul commented Dec 15, 2021

athul commented Dec 15, 2021

gbmor left a comment

Choose a reason for hiding this comment

gbmor Dec 15, 2021

Choose a reason for hiding this comment

devblin Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

gbmor Dec 17, 2021

Choose a reason for hiding this comment

devblin commented Dec 16, 2021

athul left a comment

Choose a reason for hiding this comment

devblin commented Dec 18, 2021

athul commented Dec 19, 2021

devblin commented Dec 19, 2021

athul commented Dec 21, 2021

gbmor left a comment •

edited

Loading

devblin Dec 16, 2021 •

edited

Loading