[scd] oir upsert: factor out extension and validation of existing subscription #1098
Conversation
Shastick
force-pushed
the
oir-upsert-refactor-2b-use-adapt-existing
branch
2 times, most recently
from
3251ac8 to
884cce3
Compare
There was a problem hiding this comment.
TBH I'm a bit less convinced by factoring away this function, its abstraction isn't very clear, its name and arguments are an indication of that.
I'm also not sure it is worth spending the time refactoring it more.
Now I don't think it makes anything worse than it is currently either.
So I'm OK merging the PR (after addressing the comment), but also OK not merging it, up to you.
| if err != nil { | ||
| return stacktrace.Propagate(err, "Failed to update existing Subscription") | ||
| } | ||
| // Propagating the code here is important for properly handling all failure cases in the caller |
There was a problem hiding this comment.
Doesn't the simple Propagate does that in any case? I thought the error code would be unwrapped at the top of the stack.
There was a problem hiding this comment.
You're right: if the passed cause has a code, it will be re-used.
(But, for the record, if it has none, there will be no attempt to go look a level deeper for a code)
| return nil, stacktrace.NewError("Cannot depend on a non-specified Subscription") | ||
| } | ||
|
|
||
| sub, err := r.GetSubscription(context.Background(), params.subscriptionID) |
There was a problem hiding this comment.
Context of function must be used here.
Ideally, I'd like to have the whole body of the action ( In this case, the main thing I'd like to hide away is the extension of the implicit subscription. In that regard, I'll make a new proposal where that method is split in two. |
Shastick
force-pushed
the
oir-upsert-refactor-2b-use-adapt-existing
branch
2 times, most recently
from
85f32c9 to
6f7b964
Compare
|
@mickmis the PR now contains a proposal that hides away less logic |
Shastick
force-pushed
the
oir-upsert-refactor-2b-use-adapt-existing
branch
from
6f7b964 to
9fc5b01
Compare
Shastick
force-pushed
the
oir-upsert-refactor-2b-use-adapt-existing
branch
from
9fc5b01 to
af03e7e
Compare
Another step towards #1088