Skip to content

Feat/add binary comparison#1

Merged
exyi merged 5 commits intoexyi:masterfrom
Michal-MK:feat/add-binary-comparison
Jan 27, 2023
Merged

Feat/add binary comparison#1
exyi merged 5 commits intoexyi:masterfrom
Michal-MK:feat/add-binary-comparison

Conversation

@Michal-MK
Copy link
Copy Markdown
Contributor

  • added net7.0 target
  • added CheckBinary function to well... check binary content
  • added GetOldBinaryContent which avoids the terminal and string.Join("\n")
  • added CheckOutputBinaryCore which avoids string.Replace and other undesirable operations on binary files

- added net7.0 target
- added CheckBinary function to well... check binary content
- added GetOldBinaryContent which avoids the terminal and string.Join("\n")
- added CheckOutputBinaryCore which avoids string.Replace and other undesirable operations on binary files
Copy link
Copy Markdown
Owner

@exyi exyi left a comment

Choose a reason for hiding this comment

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

Cool, thank you, this feature makes sense to me. I think I found a mistake GetOldBinaryContent, other than that it's ok.

Could you maybe add a unit test demonstrating that it works? (in the exampleTest folder, nothing complex, just new byte[] { 0, 1, 2 } or something)

Comment thread src/OutputChecker.cs Outdated

private byte[] GetOldBinaryContent(string file)
{
return File.ReadAllBytes(file);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This has to take the staged value from git (same as the GetOldContent function does it). Otherwise, on the first run the test will fail, but overwrite the file with the new (probably broken) value. Next run will read the already broken file, and pass. If we read the file from git stage, the user has to explicitly use git add to overwrite the file, so just running the tests mutliple times won't fix the test failure.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

git cat-file should just return the binary data to stdout, these low level git commands usually don't care about text/binary distinctions

- added BinaryTests to varify functionality
- added test Image.png as the base file and respective test outputs for each function
@Michal-MK
Copy link
Copy Markdown
Contributor Author

Michal-MK commented Jan 27, 2023

I had to restructure the code a bit as StreamReader.ReadLine will not work well for binary files (stripping '\n', '\r' and '\r\n' and now which one was it...?). There is a minor quirk with the CheckModifiedBinaryData test as it modifies the test file with the CheckBinary function call and VS is eager to reload it... so a popup has to be dismissed. But the metadata is not changed so nothing has to be staged in.

@exyi
Copy link
Copy Markdown
Owner

exyi commented Jan 27, 2023

Oh, I didn't anticipate .NET Process only allows reading through the StreamReader 🤦 I think reading it using ASCII encoding into chars is also going to cause problems (like interpreting all >127 bytes as ?), https://stackoverflow.com/questions/4143281/capturing-binary-output-from-process-standardoutput claims the only reasonable way is using StreamReader.BaseStream. Would you mind to try that out?

There is a minor quirk with the CheckModifiedBinaryData test as it modifies the test file with the CheckBinary function call and VS is eager to reload it

I think it's because we always load the byte array full of ?, so it thinks the file has changed and always overwrites it. Normally, the code avoids writing into the file if it didn't change.

- added more tests to check characters converted to bytes with different encoding
@Michal-MK
Copy link
Copy Markdown
Contributor Author

Right, I reverted the encoding change and added two more tests to check how it will behave with UTF-8 and ASCII. Seems good from the very limited test suite ;) Also the popup went away after restarting VS, so I do not know if/when it will come back.

@Michal-MK
Copy link
Copy Markdown
Contributor Author

The popup only shows when I have the file in question open in VS. So that is not an issue as it would popup regardless. I also forgot to revert the file back to the original content so that is fixed now.

@exyi
Copy link
Copy Markdown
Owner

exyi commented Jan 27, 2023

Great, thank you!

I'm just wondering why I can't run github Actions on the PR. I will "solve" the issue by merging it, but if you'd know by chance, let me know.

The popup only shows when I have the file in question open in VS

Ok then, I guess that's expected when the file changes.

Seems good from the very limited test suite ;)

I'm afraid you added more tests than I had for the string comparisons 😁

@exyi exyi merged commit 667a421 into exyi:master Jan 27, 2023
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