Skip to content

Return packed array #4156

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Aug 1, 2016
Merged

Return packed array #4156

merged 5 commits into from
Aug 1, 2016

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Jul 28, 2016

This make all pack functions return the packed array. This helpful if I want to do something like

var myObject = {
   position: Cartesian3.pack(position, new Array(3));
} 

I also think it's all around more intuitive. I know we do this a lot, but to me this syntax is weird

Cartesian3.pack(position, array);

And this makes much more sense

array = Cartesian3.pack(position, array);

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 31, 2016

This is also well-aligned with math functions like Cartesian3.add.

Can you update reference doc, update CHANGES.md, and add full unit tests?

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 1, 2016

@pjcozzi updated CHANGES.md and added doc

I think all classes that have pack use createPackableSpecs to create their pack specs. Since I check for the array to be returned there, I think that should cover all of the functions I changed

@@ -14,7 +14,8 @@ define([

it(namePrefix + ' can pack', function() {
var packedArray = [];
packable.pack(instance, packedArray);
packedArray = packable.pack(instance, packedArray);
expect(packedArray).toBeDefined();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than re-assigned packagedArray, create a new variable called returnedArray and do expect(returnedAraray).toBe(packedArray) in order to verify that's what is actually returned.

@mramato
Copy link
Contributor

mramato commented Aug 1, 2016

Except for that one comment, this is fine with me. Thanks, @hpinkos!

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 1, 2016

Thanks @mramato! Updated

@pjcozzi pjcozzi merged commit 552c486 into master Aug 1, 2016
@pjcozzi pjcozzi deleted the packReturnArray branch August 1, 2016 16:03
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants