Add method to reorder steps in sequencer object#1626
Add method to reorder steps in sequencer object#1626rishabhshuklax wants to merge 2 commits intopubliclab:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1626 +/- ##
==========================================
- Coverage 66.67% 65.23% -1.45%
==========================================
Files 130 132 +2
Lines 2686 2750 +64
Branches 433 439 +6
==========================================
+ Hits 1791 1794 +3
- Misses 895 956 +61
🚀 New features to boost your workflow:
|
harshkhandeparkar
left a comment
There was a problem hiding this comment.
I think the old code has non-descriptive variables and is, well, old. Can we at least make the new additions better if not change the old code?
I think we should make a seperate issue to change these names in a single PR as changing the names on the newer code will make it inconsistent. |
| function reorderSteps(source, dest) { | ||
| var this_ = (this.name == 'ImageSequencer') ? this : this.sequencer; | ||
| var args = []; | ||
| var json_q = {}; |
There was a problem hiding this comment.
I meant this btw, can these be named something better?
There was a problem hiding this comment.
Are that's what I said in the file these names are used in the other functions as well so it'd be better to change it all together so as to avoid any inconsistency.
image-sequencer/src/ImageSequencer.js
Lines 127 to 131 in fa96a7a
There was a problem hiding this comment.
So you basically don't know what they even mean? 😂
There was a problem hiding this comment.
It's fine. APPROVED lol.
There was a problem hiding this comment.
Hey lets be nice here! I am probably guilty of the ambiguous naming after all :-) so I can take the blame.
I agree though that let's change it in a consistent manner so as not to confuse folks. And if there are any opportunities to reduce repetitive code, that's great too!
There was a problem hiding this comment.
Oh sorry. I was just joking anyway, you are lucky that I didn't build sequencer's backend 😂
jywarren
left a comment
There was a problem hiding this comment.
This is a cool one! I'd like to request:
- a good test demonstrating its use and guaranteeing it against future breakage
- a line in the README describing the function and how to use it.
Thank you!!!! No rush, just looking for what we should start requiring for all changes moving forward!
Fixes #1621
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm run test-all@publiclab/is-reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!