Add edit flow for Helix #5394
Add edit flow for Helix #5394
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
@franknoirot @jtran @nadr0 Alright adding you guys for review a little early here cause there are a few not-so-pretty things in this code but I'd like your input before spending too much time on it alone! But outside of these small-ish things this was super easy to implement to way to go on laying out the groundwork for people to run on it 👟 |
| return baseCommand | ||
| } | ||
|
|
||
| // TODO: find a way to loop over the arguments while keeping it safe |
There was a problem hiding this comment.
Input welcome here. Lots of arguments here. 4 kcl, 2 options
There was a problem hiding this comment.
Interesting! Great test case for making this code more concise with helper functions and reorganization, thanks for doing this. I'm going to see if we can make TS happy with an Array.every over all the keys we need to be present in operation.labeledArgs, then try to loop over them in one go.
| operation.labeledArgs.axis.sourceRange[0], | ||
| operation.labeledArgs.axis.sourceRange[1] | ||
| ) | ||
| .replaceAll("'", '') // TODO: fix this crap |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5394 +/- ##
=======================================
Coverage 86.02% 86.02%
=======================================
Files 95 95
Lines 35754 35754
=======================================
Hits 30757 30757
Misses 4997 4997
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
We good to move forward on this? |
|
@lf94 it's far from perfict as is but nothing that can't be improved later |
|
As long as it's "stable" I don't mind it being a weak feature, merged and improved than forever awaited to be perfect |
|
@lf94 Yup. @franknoirot lmk if you want this merged as is or prefer to have the improvements we talked about |
Fixes #5392