Skip to content

v.util: improve code related to diff tool specified via environment, add check if the diff tool exists - #21240

Merged
spytheman merged 1 commit into
vlang:masterfrom
ttytm:util/env-diff-cmd
Apr 11, 2024
Merged

spytheman merged 1 commit into
vlang:masterfrom
ttytm:util/env-diff-cmd

Conversation

@ttytm

@ttytm ttytm commented Apr 9, 2024

Copy link
Copy Markdown
Member

The PR works on the util.diff module's code related to the VDIFF_TOOL environment variable.
It cleans up to help with maintainability and slightly extends by adding an error that should improve usability.

Regarding the changes:
At the beginning of the function we already checked if there is an env_difftool and return early. How env_difftool was used after this in the function, was quasi dead code.

if env_difftool != '' {
return '${env_difftool} ${env_diffopts}'
}

Then, instead of just returning the strings specified in the environment string it is also ensured here that specified command is executable. If not an error is returned to the user. E.g. when it isn't installed or if a typo or something.

@ttytm ttytm changed the title util: improve code related to diff to specified via environment, add check if command exists util: improve code related to diff tool specified via environment, add check if command exists Apr 9, 2024

@spytheman spytheman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Excellent work.

@spytheman spytheman changed the title util: improve code related to diff tool specified via environment, add check if command exists v.util: improve code related to diff tool specified via environment, add check if the diff tool exists Apr 11, 2024
@spytheman
spytheman merged commit fa321ed into vlang:master Apr 11, 2024
@ttytm
ttytm deleted the util/env-diff-cmd branch April 11, 2024 14:29
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.

2 participants