Skip to content

Create the side-by-side option (-y) feature for the diff command (Incomplete) #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sami-daniel
Copy link

@sami-daniel sami-daniel commented Apr 22, 2025

  • Create the function, in the utils package, limited_string that allows you to truncate a string based on a delimiter (May break the encoding of the character where it was cut)

  • Create tests for limited_string function

  • Add support for -y and --side-by-side flags that enables diff output for side-by-side mode

  • Create implementation of the diff -y (SideBySide) command, base command for sdiff, using the crate diff as engine. Currently it does not fully represent GNU diff -y, some flags (|, (, ), , /) could not be developed due to the limitation of the engine we currently use (crate diff), which did not allow perform logic around it. Only the use of '<' and '>' were enabled.

TL;DR

A new limited_string function was added to the utils package, allowing strings to be truncated based on a delimiter (note: this may break character encoding at the cut point). Unit tests were created to ensure correct behavior of the function.

Support for the -y and --side-by-side flags was added to enable side-by-side diff output. The SideBySide implementation, serving as the base for the sdiff command, was developed using the diff crate as the comparison engine. Due to limitations of the crate, only the < and > markers are currently supported—other markers like |, (, ),\ and / could not be implemented.

Clarification
The main goal of the limited_string function is to simplify and standardize the process of truncating strings for everyone working on the project. The idea was to provide a utility that anyone could use without having to reimplement or worry too much about string manipulation logic. However, a known limitation of the current implementation is that it can break character encoding, especially when the cut happens mid-way through a multi-byte character (e.g., in UTF-8).

I am open to suggestions on how to improve or safely handle encoding, whether by detecting boundaries or using a safer string-splitting strategy that respects character integrity.

Regarding the SideBySide implementation: I recognize that the output and edit script provided by the diff crate is limited. These limitations currently prevent us from supporting additional characters such as |, (, ), , or / that are used in GNU diff -y to more accurately reflect changes in lines.

For this initial version, I think the use of < and >, are sufficient for a first pass of side-by-side comparison. Moving forward, I did like to open a follow-up issue where we can evaluate better strategies (possibly even considering another diff engine) to handle richer diff representations and support for the missing symbols.

Suggestions and ideas are very welcome!

…omplete).

- Create the function, in the utils package, limited_string that allows you to truncate a string based on a
delimiter (May break the encoding of the character where it was cut)

- Create tests for limited_string function

- Add support for -y and --side-by-side flags that enables diff output for side-by-side mode

- Create implementation of the diff -y (SideBySide) command, base command for sdiff, using the crate
diff as engine. Currently it does not fully represent GNU diff -y, some flags (|, (, ), , /) could
not be developed due to the limitation of the engine we currently use (crate diff), which did not
allow perform logic around it. Only the use of '<' and '>' were enabled.

- Create tests for SideBySide implementation
Copy link

codecov bot commented Apr 22, 2025

Codecov Report

Attention: Patch coverage is 39.83740% with 74 lines in your changes missing coverage. Please review.

Project coverage is 83.68%. Comparing base (c02273c) to head (1374279).

Files with missing lines Patch % Lines
src/side_diff.rs 0.00% 68 Missing ⚠️
src/params.rs 28.57% 5 Missing ⚠️
src/diff.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #117      +/-   ##
==========================================
- Coverage   84.66%   83.68%   -0.99%     
==========================================
  Files          12       13       +1     
  Lines        5627     5748     +121     
  Branches      476      479       +3     
==========================================
+ Hits         4764     4810      +46     
- Misses        848      921      +73     
- Partials       15       17       +2     
Flag Coverage Δ
macos_latest 83.71% <39.83%> (-0.95%) ⬇️
ubuntu_latest 83.81% <39.83%> (-0.96%) ⬇️
windows_latest 22.98% <1.62%> (-0.49%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sami-daniel
Copy link
Author

I'll fix the alerts before reopening the PR, 😁

@sami-daniel sami-daniel reopened this Apr 22, 2025
@sami-daniel
Copy link
Author

Is done : )

@oSoMoN
Copy link
Collaborator

oSoMoN commented May 6, 2025

Hi Sami. Thanks for your work on this feature!
I don't have time to spare right now to review it, but please be assured that I will as soon as I can.

@sylvestre sylvestre requested a review from Copilot May 6, 2025 09:17
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new string truncation utility (limited_string) with unit tests, and implements a side-by-side diff mode using the diff crate for output.

  • Added limited_string utility in the utils package with tests
  • Implemented the -y/--side-by-side flags and corresponding diff output in side_diff
  • Updated module re-exports and parameter parsing to support the new feature

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/utils.rs Added limited_string function and tests
src/side_diff.rs Implemented side-by-side diff output using diff crate
src/params.rs Added parameter handling for -y/--side-by-side flag
src/main.rs Integrated side_diff module into main diff workflow
src/lib.rs Re-exported side_diff diff function with an alias
src/diff.rs Updated to include the new side_diff option
Comments suppressed due to low confidence (2)

src/side_diff.rs:22

  • There are multiple spelling issues: 'nescessary' should be 'necessary' and 'lenght' should be 'length'; please correct these in the comment.
    // recalculate how many spaces are nescessary, cause we need to take into consideration the lenght of the word before print it.

src/lib.rs:15

  • [nitpick] The re-export alias 'side_by_syde_diff' appears to have a typo; consider renaming it to 'side_by_side_diff' for clarity.
pub use side_diff::diff as side_by_syde_diff;

English typos

Co-authored-by: Copilot <[email protected]>
sami-daniel and others added 2 commits May 6, 2025 06:31
Rename export to side_by_side_diff

Co-authored-by: Sylvestre Ledru <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants