Skip to content
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

Extract ldiff display logic to reuse it as a lib #103

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

Conversation

Annih
Copy link
Contributor

@Annih Annih commented Jan 28, 2025

The ldiff "display" logic was too tightly coupled with the command line program. See #46.
Let's just extract it in a distinct class method accepting all the necessary parameters.

One can now use:

Diff::LCS::Ldiff.diff?(
        Diff::LCS::Ldiff::InputInfo.new('file1'),
        Diff::LCS::Ldiff::InputInfo.new('file2'),
        :unified,            # or :ed or :context or :report
        $stdout,           # or a ref to [] or to a StringIO or a File
        binary: nil,        # or true or false if you want to be explicit
        lines: 3)            # to customize the number of context lines

The result will be true in case of diff and false otherwise.
The output if any, will be appended to the output parameter.

I also used the opportunity to refacto the format handling to use a "nice" switch instead of multiple if statements.

Note: I didn't add test here, as normally the ldif bin specs should cover the critical use cases.

This should fix #46 🤞🏻

Annih added 2 commits January 28, 2025 20:47
The ldiff "display" logic was too tightly coupled with the command line
program. Let's just extract it in a distinct class method accepting all
the necessary parameters.

One can now use
  Diff::LCS::Ldiff.diff?(
    Diff::LCS::Ldiff::InputInfo.new('file1'),
    Diff::LCS::Ldiff::InputInfo.new('file2'),
    :unified, # or :ed or :context or :report
    $stdout, # or a ref to [] or to a StringIO or a File
    binary: nil, # or true or false if you want to be explicit
    lines: 3) # to customize the number of context lines

The result will be `true` in case of diff and `false` otherwise.
The output if any, will be appended to the output parameter.
Comment on lines +22 to +26
InputInfo = Struct.new(:filename, :data, :mtime) do
def initialize(filename)
super(filename, ::File.read(filename), ::File.stat(filename).mtime)
end
end
Copy link
Owner

Choose a reason for hiding this comment

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

This should be a namespaced constant to not leak constants to the global namespace, and I would replace :mtime with :stat and just grab .mtime from the stat when we need it (the rest of the stat data may be useful for others in the future).

Suggested change
InputInfo = Struct.new(:filename, :data, :mtime) do
def initialize(filename)
super(filename, ::File.read(filename), ::File.stat(filename).mtime)
end
end
Diff::LCS::Ldiff::InputInfo = Struct.new(:filename, :data, :stat) do
def initialize(filename)
super(filename, ::File.read(filename), ::File.stat(filename))
end
end

Comment on lines +112 to +115
if binary.nil?
old_txt = info_old.data[0, 4096].scan(/\0/).empty?
new_txt = info_new.data[0, 4096].scan(/\0/).empty?
binary = !old_txt || !new_txt
Copy link
Owner

Choose a reason for hiding this comment

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

I have merge #104 which will hopefully rebase into this nicely.

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.

Extract diff display logic from Diff::LCS::Ldiff
2 participants