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

Fix TestFilesystem::read_dir_diff_paths #42

Merged
merged 1 commit into from
May 11, 2024

Conversation

emesterhazy
Copy link
Collaborator

This function collects the files present in the left and right directories. It should return the path suffix with the left and right prefix stripped. This is necessary because process_opts will add the left and right prefix back to the paths returned by read_dir_diff_paths. If we don't strip the prefix, we will end up with invalid paths where the prefix is duplicated.

This went unnoticed since the current tests don't diff directories. I will add a test that diffs directories in a subsequent commit.

@emesterhazy
Copy link
Collaborator Author

Sorry, I accidentally closed this PR's predecessor by deleting the branch on my fork.

#38

Copy link
Owner

@arxanas arxanas left a comment

Choose a reason for hiding this comment

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

It might be that the TestFilesystem interface is bad. We could also store the left and right files in separate variables, for example, and union them together to calculate read_dir_diff_paths, instead of the thing we're implicitly doing here where

src/scm_diff_editor.rs Outdated Show resolved Hide resolved
This function collects the files present in the `left` and `right` directories.
It should return the path suffix with the `left` and `right` prefix stripped.
This is necessary because `process_opts` will add the `left` and `right` prefix
back to the paths returned by `read_dir_diff_paths`. If we don't strip the
prefix, we will end up with invalid paths where the prefix is duplicated.

This went unnoticed since the current tests don't diff directories. I will add
a test that diffs directories in a subsequent commit.
@emesterhazy emesterhazy force-pushed the push-snkowsyywvlm branch from 0c23fc3 to 52a2aa8 Compare May 9, 2024 18:11
@arxanas arxanas merged commit ba2744d into arxanas:main May 11, 2024
2 checks passed
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