refactor: remove context.Background() references from triggers command#47
refactor: remove context.Background() references from triggers command#47
Conversation
mwbrooks
added
code health
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
- Coverage 62.90% 62.88% -0.02%
==========================================
Files 210 210
Lines 22149 22147 -2
==========================================
- Hits 13932 13928 -4
- Misses 7129 7132 +3
+ Partials 1088 1087 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
A few comments to help guide the reviewer 🥾
| } | ||
|
|
||
| err = validateCreateCmdFlags(clients, &createFlags) | ||
| err = validateCreateCmdFlags(ctx, clients, &createFlags) |
There was a problem hiding this comment.
note: For the most part, this PR updates 2 functions to accept ctx context.Context. So, you'll see a lot of changes that just add ctx as an argument.
| func getFullyQualifiedTriggerFilePaths(clients *shared.ClientFactory, triggerPaths []string) ([]string, error) { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
note: This is 1st key change, where we remove ctx := context.Background() and replace wit with a ctx context.Context function argument.
| func maybeSetTriggerDefFlag(clients *shared.ClientFactory, createFlags *createCmdFlags) error { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
note: This is the 2nd key change, where we also replace ctx := context.Background() with the ctx context.Context function argument.
There was a problem hiding this comment.
🗣️ This is a ramble, but I'm wondering if we want to explore using ctx to timeout certain prompts?
IIRC confusion has appeared when inputs aren't obvious, and a timeout with error might be clear that something was expected. No blocker for this PR though!
There was a problem hiding this comment.
We can talk about it more, but it may be a fine line between knowing when someone is taking their time thinking vs unaware of the prompt and waiting.
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@mwbrooks LGTM and it's nice to find related changes in one set of changes.
All of the updates and even a line removed are solid. I did leave one comment unrelated to these chagnes that ctx has me excited to consider. These improvements I think are making changes like that even possible - cheers aplenty! 🎉
|
|
||
| for name, tt := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| ctx := slackcontext.MockContext(t.Context()) |
There was a problem hiding this comment.
I'm really enjoying the slackcontext.MockContext() function. It's been very handy to consistently sprinkle a mocked context into our tests.
| func maybeSetTriggerDefFlag(clients *shared.ClientFactory, createFlags *createCmdFlags) error { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
🗣️ This is a ramble, but I'm wondering if we want to explore using ctx to timeout certain prompts?
IIRC confusion has appeared when inputs aren't obvious, and a timeout with error might be clear that something was expected. No blocker for this PR though!
|
Thanks for the quick review @zimeg! I imagine these context PRs are getting a little stale to read through 😆 |
2 participants
Summary
Context delivery, get it while it's hot! 💁🍕
This pull request updates the
triggercommand to no longer usecontext.Background(). Not too exciting, but I've separated this into a single PR because its changes span a few filesRequirements