Closed
Bug 779391
Opened 12 years ago
Closed 12 years ago
Move deepCopy to CommonUtils
Categories
(Cloud Services :: Firefox: Common, defect)
Cloud Services
Firefox: Common
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla17
People
(Reporter: gps, Assigned: gps)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
8.83 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
This is required for the upcoming repositories patch. deepCopy was previously defined in head_helpers and used across tests. I need to use deepCopy from a testing-only JS module. I moved it to an importable module that seemed to make sense. I suppose I could move it to a testing-only JS module, since no release code uses it. I'll give reviewer discretion. I took the opportunity to refactor the implementation slightly. (Mostly to meet style guidelines.)
Attachment #647807 -
Flags: review?(rnewman)
Comment 1•12 years ago
|
||
Comment on attachment 647807 [details] [diff] [review] Move deepCopy into module, v1 Review of attachment 647807 [details] [diff] [review]: ----------------------------------------------------------------- Slightly stricter than usual, as we're lifting it into a more visible place. ::: services/common/tests/unit/test_utils_deepCopy.js @@ +13,5 @@ > + do_check_neq(ret.a[1], thing.a[1]); > + do_check_eq(ret.o.foo, thing.o.foo); > + do_check_eq(ret.o.bar[0], thing.o.bar[0]); > + do_check_eq(ret.a[0], thing.a[0]); > + do_check_eq(ret.a[1].bar, thing.a[1].bar); Please add a test for deepCopy on an array. I know it's recursive, but the top-level version deserves coverage IMO. ::: services/common/utils.js @@ +479,5 @@ > + if (Array.isArray(thing)) { > + ret = []; > + for (let element of thing) { > + ret.push(this.deepCopy(element, noSort)); > + } Early return, avoid the `else` and the free-standing definition of `ret`. @@ +484,5 @@ > + } else { > + ret = {}; > + let props = [p for (p in thing)]; > + > + if (!noSort){ Spacing.
Attachment #647807 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 2•12 years ago
|
||
I moved deepCopy to a testing-only JS module. Hopefully this comes with a less stringent review.
Assignee: nobody → gps
Attachment #647807 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #648029 -
Flags: review?(rnewman)
Assignee | ||
Comment 3•12 years ago
|
||
Added missing file to patch.
Attachment #648029 -
Attachment is obsolete: true
Attachment #648029 -
Flags: review?(rnewman)
Attachment #648033 -
Flags: review?(rnewman)
Comment 4•12 years ago
|
||
Comment on attachment 648033 [details] [diff] [review] Move deepCopy into module, v3 Review of attachment 648033 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/common/utils.js @@ +464,5 @@ > } > > return new BinaryInputStream(stream).readBytes(count); > }, > + *chop*
Attachment #648033 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/services/services-central/rev/73d5011605a6
Whiteboard: [fixed in services]
Updated•12 years ago
|
Whiteboard: [fixed in services] → [fixed in services][qa-]
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/73d5011605a6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•