Fix relative paths failing to resolve in diffs #11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous implementation used git difftool to automatically run a script against the files that had changed. git difftool automatically created copies of the files being diffed in a temporary location, but did not copy rest of the project, causing relative paths to no longer resolve. This change removes the use of git difftool, and instead exports a copy of the repo at the pull request target before invoking the diff generator on the changed VIs. This allows relative paths to be resolved correctly, leading to nicer diffs. This change also enables generating diffs for .vit and .vim files.
|
I did some additional configuration work on one of the custom devices (making it pull in the testing tools through a submodule) to demonstrate these changes working. Here is a diff with the testing tools being loaded from a submodule and resolving correctly. |
Previously we were only importing it on error, since this script was called once per file by git difftool. Now that it is only called once, we should import traceback at the top level.
This is cleaner than changing the working directory of the parent process twice.
buckd
approved these changes
Nov 20, 2018
mshafer-NI
reviewed
Nov 20, 2018
| traceback.print_exc() | ||
|
|
||
| with open(working_dir + "\\diff_failures.txt", "a+") as file: | ||
| file.write(vi1 + "\n") | ||
| with open(output_dir + "\\diff_failures.txt", "a+") as file: |
| # and the temporary directory can be returned directly | ||
| for root, dirs, files in os.walk(directory.name): | ||
| for file in files: | ||
| os.chmod(root + "/" + file, stat.S_IWRITE) |
|
|
||
| with export_repo(target_branch) as directory: | ||
| for status, filename in diffs: | ||
| if status == "A": |
There was a problem hiding this comment.
"A" ?? this should probably be an enum (if status == difModes.Add.value)
There was a problem hiding this comment.
I was debating between adding a type and leaving it as a string. We're parsing this from git's output, and we get them back as "A" and "M". Adding a type would make it a little more robust. Good catch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this Pull Request accomplish?
Fix diffs not resolving relative paths properly.
The previous implementation used git difftool to automatically run a
script against the files that had changed. git difftool automatically
created copies of the files being diffed in a temporary location, but
did not copy rest of the project, causing relative paths to no longer
resolve.
This change removes the use of git difftool, and instead exports a copy
of the repo at the pull request target before invoking the diff
generator on the changed VIs. This allows relative paths to be resolved
correctly, leading to nicer diffs.
This change also enables generating diffs for .vit and .vim files. Fixes #10.
This change also makes running the diffing code locally easier, by pushing
environment variable handling into the Groovy layer and adding argument
parsing to the Python layer.
Why should this Pull Request be merged?
This makes the diffs look a lot nicer, and be significantly more useful.
What testing has been done?
Building a custom device pull request: http://coordinator/job/VeriStand/job/rtzoeller/job/niveristand-chassis-timesync-custom-device/view/change-requests/job/PR-2/18/
Resulting diff bot post: https://github.com/rtzoeller/niveristand-chassis-timesync-custom-device/pull/2