search

ReviewNB / support

Issues and feature requests for ReviewNB
https://reviewnb.com
59 stars 8 forks source link

Diff fails to detect equal lines #86

Closed DomNomNom closed 3 years ago

DomNomNom commented 3 years ago

Example: https://github.com/DomNomNom/reviewnb-diff-bug-report/pull/1 https://app.reviewnb.com/DomNomNom/reviewnb-diff-bug-report/pull/1/

In this example, the ReviewNB diff shows that the entire notebook has been deleted and replaced with the new version. This is a technically valid diff but exceedingly unhelpful as it makes it seem that the parts that remained the same have changed.

This notebook is significant as it is backing https://docs.q-ctrl.com/boulder-opal/user-guides/automate-closed-loop-hardware-optimization-of-quantum-devices

amit1rrr commented 3 years ago

@DomNomNom Thank you for reporting & providing us with a reproducible example. It helps us a lot.

If the diff is too large (many cells splits, merged, deleted) then our algorithm can't detect line-by-line diff and we fall back on simple diff which is what you're seeing. This happens rarely & we actively look at these cases to improve our diff'ing algorithm.

Thanks to your example, we drove deeper into this diff and we were able to fix the underlying cause. You can now see that the diff works correctly on line-by-line basis. Of course, there could be some other rare instances where we'd need to fall back on simple diffs but we're always looking to improve upon these cases. Closing this issue since this particular PR diff is fixed.

DomNomNom commented 3 years ago

Thanks, works correctly in the internal repo now too. :D