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

[FEATURE REQUEST] idiff "directory mode" #4009

Closed
lgritz opened this issue Oct 9, 2023 · 6 comments · Fixed by #4015
Closed

[FEATURE REQUEST] idiff "directory mode" #4009

lgritz opened this issue Oct 9, 2023 · 6 comments · Fixed by #4015
Assignees
Labels
enhancement Improvement of existing/working features. feature request good first issue Good one-day project for beginners without much knowledge of the code base.

Comments

@lgritz
Copy link
Collaborator

lgritz commented Oct 9, 2023

On Unix/etc command line, the "diff" utility lets you say

diff path/to/file1.txt anotherpath/toanother/file2.txt

but also if you

diff path/to/file1.txt pathto/dir

where pathto/dir is the name of a directory, this is shorthand for

diff path/to/file1.txt pathto/dir/file1.txt

That is, you can just specify a directory as the second argument, and it knows that you mean to diff against a file of the same filename, in that other directory.

I would like our idiff utility to work the same way. You currently have to give both full paths, but many times I've wished you could give just a directory for the second name, and it would know what you meant and behave like diff users expect.

@lgritz lgritz added enhancement Improvement of existing/working features. feature request good first issue Good one-day project for beginners without much knowledge of the code base. labels Oct 9, 2023
@davvid
Copy link
Contributor

davvid commented Oct 10, 2023

Hi Larry, I'd like to add this feature. Feel free to assign this to me. I actually already have this working locally but I might wait until the 12th before submitting the PR to help batch things up.

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 10, 2023

I think you're the second person to say the same thing. But he seems to have deleted his comment here, so you're the winner.

@CheeksTheGeek
Copy link
Collaborator

CheeksTheGeek commented Oct 10, 2023

Hi Davvid, it was me who deleted the comment, lol. I got all of it working, but was under the confusion that we weren't supposed to work on it before DevDays.
I'll pick up something else, so best of luck for DevDays 2023!

@davvid
Copy link
Contributor

davvid commented Oct 10, 2023

Thanks @CheeksTheGeek I'll loop you into the code review when I submit my update on Thursday so we can have two pairs of eyes on the changes.

Our code is somewhat similar, but also slightly different so we can chat about the differences once it's posted.

@CheeksTheGeek
Copy link
Collaborator

Okayy!

davvid added a commit to davvid/OpenImageIO that referenced this issue Oct 12, 2023
The Unix "diff" utility allows you to run:

    diff path/to/file1.txt path/to/dir

When "diff" sees a directory it appends "file1.txt" to the
directory so that this command is treated as if the user ran:

    diff path/to/file1.txt path/to/dir/file1.txt

Teach "idiff" to detect when a directory is specified and expand
the path to include the file basename from the first argument.

Closes: AcademySoftwareFoundation#4009
Signed-off-by: David Aguilar <[email protected]>
davvid added a commit to davvid/OpenImageIO that referenced this issue Oct 12, 2023
Let users know that the 2nd argument to "idiff" can be either an image
filename or a directory.

Related-to: AcademySoftwareFoundation#4009
Helped-by: @CheeksTheGeek
Signed-off-by: David Aguilar <[email protected]>
@davvid
Copy link
Contributor

davvid commented Oct 12, 2023

@CheeksTheGeek I opened #4015 with these updates. I know you closed it but I'll share code review notes on your commit above shortly for educational purposes.

davvid added a commit to davvid/OpenImageIO that referenced this issue Oct 12, 2023
Use "input1" and "input2" in the usage example to match
the description immediately below it.

Add a sentence explaining that "input2" can also be a directory.

Related-to: AcademySoftwareFoundation#4009
Signed-off-by: David Aguilar <[email protected]>
lgritz pushed a commit that referenced this issue Oct 12, 2023
…4015)

Teach `idiff` to accept a directory as the 2nd argument.

Fixes #4009

## Tests

Functional tests for `idiff` don't currently exist. Manual testing
verifies it works as advertised.

---------

Signed-off-by: David Aguilar <[email protected]>
lgritz pushed a commit to lgritz/OpenImageIO that referenced this issue Oct 14, 2023
…cademySoftwareFoundation#4015)

Teach `idiff` to accept a directory as the 2nd argument.

Fixes AcademySoftwareFoundation#4009

## Tests

Functional tests for `idiff` don't currently exist. Manual testing
verifies it works as advertised.

---------

Signed-off-by: David Aguilar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing/working features. feature request good first issue Good one-day project for beginners without much knowledge of the code base.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants