-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add method to reorder steps in sequencer object #1626
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1626 +/- ##
===========================================
+ Coverage 55.11% 65.23% +10.11%
===========================================
Files 117 132 +15
Lines 2344 2750 +406
Branches 360 439 +79
===========================================
+ Hits 1292 1794 +502
+ Misses 1052 956 -96
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant this btw, can these be named something better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
var args = []; | |
for (var arg in arguments) args.push(copy(arguments[arg])); | |
var json_q = formatInput.call(this_, args, '-'); | |
inputlog.push({ method: 'removeSteps', json_q: copy(json_q) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you basically don't know what they even mean? 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine. APPROVED lol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry. I was just joking anyway, you are lucky that I didn't build sequencer's backend 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to 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!