Added FORM to Lama2 #39
Conversation
|
Solves: #29 |
cmdgen/cmdgen.go
Outdated
| @@ -76,6 +77,12 @@ func assembleCmdString(httpv string, url string, jsonObj *gabs.Container, header | |||
| } | |||
| } | |||
|
|
|||
| if form { | |||
| for key, val := range jsonObj.Data().(*gabs.Container).ChildrenMap() { | |||
| command = append(command, "'"+key+"'='"+val.Data().(string)+"' ") | |||
There was a problem hiding this comment.
Use fmt.Sprintf to make it more readable.
cmdgen/cmdgen.go
Outdated
| @@ -107,7 +114,12 @@ func ConstructCommand(parsedInput *gabs.Container, o *lama2cmd.Opts) ([]string, | |||
| if multipart != nil { | |||
| multipartBool = true | |||
| } | |||
| form := parsedInput.S("form","value") | |||
There was a problem hiding this comment.
make lint and make gofumpt
|
Should add at least a positive & negative test/example, and demonstrate results is as expected. Check https://github.com/HexmosTech/Lama2/blob/main/tests/lama2_test.go#L70 |
cmdgen/cmdgen.go
Outdated
| @@ -107,7 +115,12 @@ func ConstructCommand(parsedInput *gabs.Container, o *lama2cmd.Opts) ([]string, | |||
| if multipart != nil { | |||
| multipartBool = true | |||
| } | |||
| form := parsedInput.S("form", "value") | |||
| formBool := false | |||
| if form != nil { | |||
There was a problem hiding this comment.
Why not do a direct comparison for a bool? (if needed through after casts)
parser/varjson.go
Outdated
| pair, e1 := p.Match([]string{"VarJSONPair"}) | ||
| if e1 != nil && !hasMultipart { | ||
| if e1 != nil && (!hasMultipart || !hasForm) { |
There was a problem hiding this comment.
Can simplify condition. What are we checking for here?
!a || !b = !(a && b)
There was a problem hiding this comment.
If either a or b is false, then the result is true.
There was a problem hiding this comment.
Need a comment explaining the whole thing with simplification.
There was a problem hiding this comment.
When i rechecked that part, Since FORM does not have file upload, the condition is not needed, since that condition is for erroring out in the event where there is no key value pairs.
multipart has files so it can be executed even without key value pairs, so that's why that condition is there only for multipart
prettify/prettify.go
Outdated
| @@ -52,6 +52,6 @@ func Prettify(parsedAPI *gabs.Container, context map[string]bool, markRange map[ | |||
| } | |||
|
|
|||
| res2 := utils.RemoveUnquotedMarker(content) | |||
| os.WriteFile(fPath, []byte(res2), 0644) | |||
| os.WriteFile(fPath, []byte(res2), 0o644) | |||
There was a problem hiding this comment.
This change needed?
According to Go By Example, no.
There was a problem hiding this comment.
It came up when i ran make gofumpt
| testutils "github.com/HexmosTech/lama2/tests/utils" | ||
| ) | ||
|
|
||
| func TestFormPositive(t *testing.T) { |
There was a problem hiding this comment.
Have we verified none of the MULTIPART cases are breaking?
Just verify how postman deals with a multipart without file attachment once. I think we should verify that case works (if a testcase not there, we can add that as well).
What type of MR is this? (check all applicable)
Description
This MR adds FORM to lama2
Solves: #29
Important files to start review from
Added tests?
Added to documentation?
If documentation update is there then run
make mkdocsonce PR is in acceptable state.