Truncate diff of very long texts if not in verbose mode (fix #12406) #12634
Conversation
|
The logic for when equal leading characters aren't skipped is now sound. |
There was a problem hiding this comment.
Hey @devdanzin!
Really sorry about the delay on this one, it fell through the cracks.
I'm afraid it needs further updates since #12766 landed, see my comments.
Also it is understandable if you would prefer to drop this instead, as it has not been given the attention it deserved and the implementation will probably be a bit more complicated to handle the new limit options.
| @@ -308,6 +311,21 @@ def _diff_text(left: str, right: str, verbose: int = 0) -> list[str]: | |||
| ] | |||
| left = left[:-i] | |||
| right = right[:-i] | |||
| shortest = min(left, right, key=lambda x: len(x)) | |||
| lines = j = start = 0 | |||
| if shortest.count("\n") >= DEFAULT_MAX_LINES: | |||
There was a problem hiding this comment.
I'm afraid this PR got outdated after #12766.
I think the path forward would be for _diff_text() to receive both "max lines" and "max chars" by parameter instead (they being int | None, with None meaning "no limits").
| @@ -308,6 +311,21 @@ def _diff_text(left: str, right: str, verbose: int = 0) -> list[str]: | |||
| ] | |||
| left = left[:-i] | |||
| right = right[:-i] | |||
| shortest = min(left, right, key=lambda x: len(x)) | |||
| lines = j = start = 0 | |||
| if shortest.count("\n") >= DEFAULT_MAX_LINES: | |||
There was a problem hiding this comment.
I'm under the impression the code can be made simpler by skipping max chars first, then dealing with max lines... 🤔
The
_diff_text_function might try to calculate diffs of huge texts, taking a very long time, even if that diff is going to be truncated in non-verbose mode. This PR adds truncation of the texts before calculating the diff in that case. New tests added, old tests pass.I'm not sure the code is correct for when equal trailing characters aren't skipped.
Happy to address any reviews or suggestions.
Closes #12406.