Basic XML modification for number-of-cells parameter#9
Conversation
gabroo
left a comment
gabroo
left a comment
There was a problem hiding this comment.
Good work, few minor comments, then we can merge this
gabroo
left a comment
gabroo
left a comment
There was a problem hiding this comment.
thanks for the updates! few quick fixes!
| } | ||
|
|
||
| // Change model path to individual simulation's modified xml | ||
| model = xmlConvert(model, out, params) |
There was a problem hiding this comment.
I think xmlConvert should have return type (string, error), see comment below
| return id, err | ||
| } | ||
|
|
||
| func xmlConvert(model string, out string, params *pb.Params) (string){ |
There was a problem hiding this comment.
change return type to (string, error) here and instead of panic below, just return the error
| model_path := out + "/barkley3d_coupling.xml" | ||
| doc.WriteToFile(model_path) | ||
|
|
||
| return model_path |
There was a problem hiding this comment.
you can return model_path, nil here
| @@ -5,7 +5,7 @@ import "common.proto"; | |||
| option go_package = ".;pb"; | |||
|
|
|||
| // Human-readable parameters for our GJSM model. | |||
There was a problem hiding this comment.
can you move this comment on top of the Params struct? then the swagger docs will show it
| } | ||
|
|
||
| // Write to output directory | ||
| model_path := out + "/barkley3d_coupling.xml" |
There was a problem hiding this comment.
use the name from model param instead of hardcoding
| } | ||
| id, err := startJob(s.c, "models/three_layer.xml", "out") | ||
|
|
||
| id, err := startJob(s.c, "models/barkley3d_coupling.xml", "out", rq) |
There was a problem hiding this comment.
can you grab the model/out strings from the RunRequest instead of hardcoding? that'll make it easier to change later
| model, out := rq.Model, rq.Out | ||
| id, err := startJob(model, out, s.jobs) | ||
| model, out, params := rq.Model, rq.Out, rq.Params | ||
| // had to change here to get this to compile even though it isn't used right now -MD |
There was a problem hiding this comment.
sure, that's fine; you can remove this comment
2 participants
No description provided.