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

v.util: ensure output of color_compare_files is colorized #21244

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Apr 9, 2024

Makes the function do what it's name implies. I was really missing this when there is just diff installed and working in the codebase.

Current Updated
Screenshot_20240409_195735 Screenshot_20240409_195741

Due to the ansi codes in the output, it will conflict with #21242. Depending on which one is resolved first conflicts in the other will need to be taken care of.

vlib/v/util/diff/diff.v Outdated Show resolved Hide resolved
@spytheman
Copy link
Member

I've reverted bfd53a9 in 056a443. The reason is that find_working_diff_command can be used to form a diff command, customizable by setting VDIFF_TOOL and VDIFF_OPTIONS, that later can be passed to util.color_compare_files:

import v.util
import v.util.diff
diff_cmd := diff.find_working_diff_command()!
res := util.color_compare_files(diff_cmd, 'v.mod', 'v2.mod')
println(res)

image

@spytheman spytheman closed this Apr 10, 2024
@ttytm
Copy link
Member Author

ttytm commented Apr 10, 2024

I think this still can be used with #21242 and that the PR does not need to be reverted or this one closed

@ttytm ttytm reopened this Apr 10, 2024
@ttytm
Copy link
Member Author

ttytm commented Apr 10, 2024

What command does it use for you? Do you have colordiff additionally installed?
It would be nice to get color with the default system diff tool as well that is just diff

@spytheman
Copy link
Member

I think this still can be used with #21242 and that the PR does not need to be reverted or this one closed

I disagree. It represents a regress in functionality, and a break on FreeBSD (whose diff tool does not support color output at all).

@spytheman
Copy link
Member

What command does it use for you? Do you have colordiff additionally installed? It would be nice to get color with the default system diff tool as well that is just diff

The list of commonly used diff commands is in known_diff_tools in vlib/v/util/diff/diff.v:18:
['colordiff', 'gdiff', 'diff', 'colordiff.exe', 'diff.exe', 'opendiff', 'code', 'code.cmd']

It would be nice, for the diff to work when it is supported, and to be customizable when the user has more sophisticated tools and needs (which currently can be done by setting VDIFF_TOOL and VDIFF_OPTIONS).

That can be done by changing find_working_diff_command, which can add options for the default commands, if it detects that they are supported.

@spytheman
Copy link
Member

image

Something like this for example works on FreeBSD too (if the user has installed diffutils).

@ttytm
Copy link
Member Author

ttytm commented Apr 10, 2024

What command does it use for you? Do you have colordiff additionally installed? It would be nice to get color with the default system diff tool as well that is just diff

The list of commonly used diff commands is in known_diff_tools in vlib/v/util/diff/diff.v:18: ['colordiff', 'gdiff', 'diff', 'colordiff.exe', 'diff.exe', 'opendiff', 'code', 'code.cmd']

It would be nice, for the diff to work when it is supported, and to be customizable when the user has more sophisticated tools and needs (which currently can be done by setting VDIFF_TOOL and VDIFF_OPTIONS).

That can be done by changing find_working_diff_command, which can add options for the default commands, if it detects that they are supported.

Yes I wanted to take the diff module there. Though thought it's organic and sophisticated doing it step by step

@ttytm ttytm marked this pull request as draft April 10, 2024 07:17
@spytheman
Copy link
Member

Do you have colordiff additionally installed?

Yes, on both Linux and on FreeBSD, but I renamed it while testing.

@ttytm
Copy link
Member Author

ttytm commented Apr 10, 2024

Awesome fix thanks 👍

@spytheman spytheman marked this pull request as ready for review April 10, 2024 11:53
@spytheman spytheman merged commit 48800d4 into vlang:master Apr 10, 2024
56 checks passed
@ttytm
Copy link
Member Author

ttytm commented Apr 10, 2024

I was sloppy here with reviewing as well. With two things

  1. From research and the tests before I knew that it won't work to put the flag at the beginning, the output will not be colorized if doing so. So the check would have to be in the color_compare function.
  2. My initial implementation was on mac which I tried to cover. It's not covered here.

Improving the module while adding test we can eventually get there I believe as both things can be solved.

@ttytm ttytm deleted the util/fix-diff-color branch April 14, 2024 06:31
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.

None yet

2 participants