fix: eds mutation

[rahulghangas]

Call to eds.row returns a slice to the underlying row/col data. Hence, when rebuilt shares are added to this slice, the underlying data is being mutated before correct validation is done. On the other hand, using eds.Row is too expensive, since it makes a copy of the underlying data. This PR fixes the first issue while avoiding expensive copying by passing the rebuilt share and/or index to relevant methods

• Fixes solveCrosswordRow/Col mutates underlying data square, when it shouldn't be #132

[codecov]

Codecov Report

Merging #134 (cda0a15) into master (80d231f) will increase coverage by 0.17%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   81.76%   81.93%   +0.17%     
==========================================
  Files           8        8              
  Lines         521      537      +16     
==========================================
+ Hits          426      440      +14     
- Misses         58       59       +1     
- Partials       37       38       +1     

Impacted Files	Coverage Δ
extendeddatacrossword.go	76.53% <87.50%> (+0.97%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[musalbas]

Is this still ready for review @rahulghangas? I recall you said you had an idea about something yesterday

[rahulghangas]

I thought about how to replace -1 with something better, but didn't come up with anything. Thoughts?

[Wondertan]

@musalbas, I liked your solution in #114 so much that removing it now while introducing more ugly branchiness in the code is disappointing, but it is what it is.

[rahulghangas]

@musalbas I think I can request only one review, so pinging you here. Do the new changes work?

[musalbas]

If we can't think of a cleaner way it's ok with me. NoShareInsertion shouldn't be exported though.

[liamsi]

LGTM. One question though: would it be too difficult to test the change in bahviour as it only affects internals of the implementation?

[rahulghangas]

The only behaviour that has changed (been corrected) is for the state of the underlying eds when trying to repair from incorrect data. I guess we should add a test for that

[liamsi]

I guess we should add a test for that

Yes, please do 👍🏼 Ideally, a test that would have failed without this fix specifically.

[adlerjohn]

[code quality, blocking] Honestly I would just make these into new functions, instead of passing in a magic value for branching logic. If rebuiltIndex is noShareInsertion, then rebuiltShare must be nil, but this isn't enforced by the compiler since it's all one function.

[rahulghangas]

Separating out leads to a lot of code duplication because noShareInsertion needs to be propagated all the way inside to the last function call. Which is why I included noShareInsertion in the first place

[evan-forbes]

is this still being worked on? if so what is left? @rahulghangas

[gitpoap-bot]

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 Celestia Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.