Refactor strings

[jelder]

@aamontal @kevinmook Everything looks good to me, but you guys are a lot more familiar with the code. Mind taking a look?

[kevinmook]

Woah, that's a lot of code. Yeah, I was planning on looking it over. I want to dig into this Jenkins thing first though

[kevinmook]

A cursory look over by me looks good, but I have no idea how the old code worked :) Really appreciate the added comments, should help a ton moving forward when we need to change this in the future. @aamontal if you want to take a quick look too, feel free to merge it in

[aamontal]

@kevinmook I took a look at the specs because the code is a bit dense for a quick glance. I noticed there was no test for something like non_conflict, conflict, non_conflict, conflict ...

Is there any other edge cases or tests you think might be useful?

[kevinmook]

Hmm, good thinking..

If "c" is a conflict and "n" is a non-conflict, maybe (without looking at what's already there):

"" "n" "c" "nc" "cn" "ncn" "cnc"

Would that cover it?

[avogel]

All those cases are now covered. Also, how should nil inputs be handled? For now they are throwing an error as merge_text immediately tries to call split on nil, and a test is checking that nil inputs raise an error.

[kevinmook]

Hmm, good question... Erroring is probably fine for now? Treating it like "" would probably be acceptable too. Anyone else have any thoughts on handling nil? /cc @aamontal @jelder?

[aamontal]

@kevinmook my thought is that its easier to send up a mistaken nil than a mistaken empty string, so nil should raise an exception. Also it's the wrong datatype /cc @jelder

[jelder]

@kevinmook Totes agree. nil is indicative of a problem, but String is a String.