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.

Sign up for GitHub

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

Jump to bottom

v.util: improve color_compare_files #21243

Merged
merged 1 commit into from
Apr 9, 2024
Merged

v.util: improve color_compare_files #21243

merged 1 commit into from
Apr 9, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Apr 9, 2024

No description provided.

@spytheman spytheman changed the title util: improve color_compare_files v.util: improve color_compare_files Apr 9, 2024
@spytheman spytheman merged commit bfd53a9 into vlang:master Apr 9, 2024
56 checks passed
@ttytm ttytm mentioned this pull request Apr 9, 2024
Merged
spytheman added a commit that referenced this pull request Apr 10, 2024
@spytheman
Revert "v.util: improve color_compare_files (#21243)"
056a443 
This reverts commit bfd53a9.
It restores the ability to use diff.find_working_diff_command() and
customise the result with VDIFF_TOOL and VDIFF_OPTIONS.
@spytheman
Copy link
Member

I've reverted the changes here, since they break the ability to customize the diff command and options by setting VDIFF_TOOL and VDIFF_OPTIONS.

@ttytm
Copy link
Member Author

ttytm commented Apr 10, 2024 •
edited
Loading

Might it be that it would work with: #21242? I mentioned it in the PR description

@spytheman
Copy link
Member

No, I've tested it.
The problem is the os.find_abs_path_of_executable(diff_cmd) or { check, which makes an early return, when the passed diff_cmd is not just a name, but something like code -d, composed from VDIFF_TOOL and VDIFF_OPTIONS .

@spytheman
Copy link
Member

I should have been more thorough when reviewing it yesterday, and just not merge it in the first place.

@ttytm
Copy link
Member Author

ttytm commented Apr 10, 2024

Good point. But easily fixed with keeping the check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spytheman spytheman spytheman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ttytm @spytheman