Reorganize marshal code.

[myronmarston]

• This appears to fix DRb error when running rspec 2.14.0.rc1 with spork rspec-core#950.
• It's better, anyway, since it bypasses the unnecessary
  shifting/unshifting for this case.

I'd love if we had a failing spec for this but I don't really understand marshal and DRB to understand how the old logic was causing this bug. This change seems logically identical but it fixes the problem :/

[myronmarston]

BTW, we should merge this into master and 2-99 as well, eventually. I want to sit on this for a bit and see if I can figure more out about it.

[alindeman]

Wow, nice debugging. Very odd problem.

[alindeman]

I agree this is better. Maybe we could write a spec that args is unchanged in the case where Space is uninitialized? It'd roughly be a regression spec, even though we don't exactly know the cause.

[myronmarston]

Maybe we could write a spec that args is unchanged in the case where Space is uninitialized? It'd roughly be a regression spec, even though we don't exactly know the cause.

That sounds good, but I'm not really sure how to do that, given that the shift/unshift should result in the same array, right? I guess we could mock it, and assert that it's the same array instance, but even though, I'm not sure how to do that given that the args array is just a by-product of our use of splats and isn't actually passed in as an argument to the method...

Do you have an idea how to write such a spec?

[myronmarston]

@alindeman -- one other thought I had...I'm thinking about removing the marshal extension in 3.0. It's only needed to work around the singleton can't be dumped error. However, stubbing or mocking a method defines a singleton method, and allowing the singleton can't be dumped error kinda makes sense to me. Plus, when you roundtrip a mocked or stubbed object through Marshal, it's not going to respond to messages in the same way, since the singleton methods can't be carried across. Our code here hides that issue.

Thoughts?

[JonRowe]

I'm in favour of removing the Marshal extension provided it doesn't surprise people, could there be an issue whereby we've messed with an objects internals to provide mocking/stubbing and they try to legitimately marshall it and it blows up?

I'm also curious about why the shift/unshift is affecting drb, could it be a referencing issue? (Are the array's memory locations different after the shift/unshift operation?) Did you try just using args[0] rather than object?

[alindeman]

I think it'd be good to try to avoid mutation here as @JonRowe says. Then we may not really need the spec that I'm talking about.

[alindeman]

Is everyone cool if I take this, remove the mutation, and merge?

[myronmarston]

Is everyone cool if I take this, remove the mutation, and merge?

Yes, as long as you test it against the original source of the problem:

https://github.com/myronmarston/recurring_select/tree/rspec-troubleshooting

I don't understand why what we had was broken and why my fix fixes it...so I don't want a refactoring happening w/o verifying that.

What branches do you plan to merge this in to?

[JonRowe]

I'm cool with this provided it passes @myronmarston's test.

[alindeman]

Opened #335 with specs. I resolved the shift, unshift issue there by simply destructuring the arguments in the method definition.

[alindeman]

Resolved with #335 :)