Conversation
rabbah
left a comment
rabbah
left a comment
There was a problem hiding this comment.
There's at least one regression so requesting changes accordingly.
debug ert remove unce
steven0711dong
force-pushed
the
triggerParameterIssue
branch
from
5511ccb to
046d6a0
Compare
commands/trigger.go
Outdated
| //then we use the old way to create the trigger. | ||
| if len(Flags.common.param) > 0 || simplestTrigger { | ||
| if len(Flags.trigger.feedParam) > 0 || len(Flags.trigger.triggerParam) > 0 { | ||
| whisk.Debug(whisk.DbgError, "User tries to mix use of --param with --feed-param and --trigger-param") |
There was a problem hiding this comment.
Is there a use case for the user trying to use trigger parameters and feed parameters together?
There was a problem hiding this comment.
Yeah, if users want to create a trigger using the new way. They will create trigger parameters using --trigger-param flag and create feed parameters using --feed-param flag. They just cannot combine --param with --feed-param or --trigger-param. The meaning is ambiguous. So we issue an error here.
commands/trigger.go
Outdated
| } | ||
| annotations := getParameters(annotationArray, true, true) | ||
|
|
||
| //simplestTrigger indicates user are creating a trigger without any feed or parameters |
There was a problem hiding this comment.
Does duplicate code exist in create and update? If so it'd be better to create functions out of the duplicate code to reduce complexity.
There was a problem hiding this comment.
I created two functions so that both create and update could use. Some other variables such as params, feed params do exist in both functions but are created differently and the creation depends on users' intention on how to issue trigger command(old way and new way). So it might make things more complicated to create functions out of duplicate code.
I have to admit that after adding the new flags, both functions become bigger but I think it might be a good idea to separate execution path for the old way of creating trigger from the new way so they do not get lumped together. I'll update the PR this afternoon.
commands/trigger.go
Outdated
| } | ||
| annotations := getParameters(annotationArray, true, true) | ||
|
|
||
| //Within this if statement, we process users' trigger command using the old way. |
There was a problem hiding this comment.
This is quite a large method with over 100 lines of code. Generally large functions are hard to follow and should be broken out into several functions.
There was a problem hiding this comment.
I can try to separate the 'old way' and 'new way' into two different functions.
commands/trigger.go
Outdated
| // Get full feed name from trigger get request as it is needed to get the feed | ||
| if retTrigger != nil && retTrigger.Annotations != nil { | ||
| fullFeedName = getValueString(retTrigger.Annotations, "feed") | ||
| // Get full feed name from trigger get request as it is needed to get the feed |
There was a problem hiding this comment.
This this update method has 200 lines of code making it extremely hard to understand quickly.
dubee
left a comment
dubee
left a comment
There was a problem hiding this comment.
Is it possible to add unit tests for these changes since there are no integration tests?
|
I've already changed the error message thing, I'll look at how to break them into small functions tomorrow |
steven0711dong
force-pushed
the
triggerParameterIssue
branch
from
861bd1b to
7fb2c36
Compare
commands/trigger.go
Outdated
|
|
||
| //if trigger contains no feed but user tries to update feed parameter, then we issue error. | ||
| if feedQualifiedName == nil && len(Flags.trigger.feedParam) > 0 { | ||
| whisk.Debug(whisk.DbgError, "Incorrect usage. trigger without a feed cannot have feed parameters") |
There was a problem hiding this comment.
This error string does not match what is in en_US.all.json.
3 participants
close #290, #293