Implement Run Script comparison

[michaelmcguire]

Issue number of the reported bug or feature request: #110

Describe your changes

• Added RunScriptComparator for doing detailed comparisons of an Xcode "Run Script" build phase (represented in XcodeProj as PBXShellScriptBuildPhase).
• Added test support for running unit tests.
• Updated the generated CommandTests after modifying the Project fixtures to have differences.

Testing performed
Added full suite of unit tests and command tests. I also ran it against the projects I'm working on (where I first noticed the problem) and saw it detected the changes.

[michaelmcguire]

After using this new functionality today, I'm wondering if perhaps the output could be improved by using an additional context value in the CompareResult to return more detailed information about the four properties that are lists of files: inputPaths, outputPaths, inputFileListPaths, and outputFileListPaths. I could use the onlyInFirst and onlyInSecond for them instead of DifferentValues. Is that something that makes sense?

If that's the case, perhaps I could open an additional Issue as an enhancement and work on that later? If that's not how it's supposed to be used, ignore all of this :)

[marciniwanicki]

Hi @michaelmcguire, many thanks for opening the PR. I've played a bit with the comparator and it worked really well 👍

Small things I'd consider.

1. Run script name
   It might be good to put the name in quotes, similarly to "Project" target, I'd hope it makes it a bit clearer to the user this is a custom/user-generated value.

instead of

RUN_SCRIPTS > "Project" target > "ShellScript"

maybe this

RUN_SCRIPTS > "Project" target > "ShellScript" build phase

2. Worth extending the p1 and p2 fixtures so they have at least 2 run script build phases. I've noticed the comparator outputs the build phases in random order (probably caused by the commonKeys: Set). Would be good to keep the order stable, sorting by name would suffice, we do the same with targets.

3. Empty characters
   I've noticed you run into w swiftlint issues on CI. It's something we should probably document. We prefer to trim white characters. Xcode can do it automatically if you have it configured.
   

4. Improving the output

After using this new functionality today, I'm wondering if perhaps the output could be improved by using an additional context value in the CompareResult to return more detailed information about the four properties that are lists of files: inputPaths, outputPaths, inputFileListPaths, and outputFileListPaths. I could use the onlyInFirst and onlyInSecond for them instead of DifferentValues. Is that something that makes sense?

That sounds like a great idea 👍 As you said, it does not need to be part of this PR, we can follow up on it.

[marciniwanicki]

Let's use Copyright 2022 Bloomberg Finance L.P..

[marciniwanicki]

Let's use Copyright 2022 Bloomberg Finance L.P..

[michaelmcguire]

Hi @michaelmcguire, many thanks for opening the PR. I've played a bit with the comparator and it worked really well 👍

You all have been excellent to work with, thanks for your help, @marciniwanicki!

Small things I'd consider.

1. Run script name
   It might be good to put the name in quotes, similarly to "Project" target, I'd hope it makes it a bit clearer to the user this is a custom/user-generated value.

instead of

RUN_SCRIPTS > "Project" target > "ShellScript"

maybe this

RUN_SCRIPTS > "Project" target > "ShellScript" build phase

Makes a lot of sense. Fixed in 6b7a7d3.

2. Worth extending the p1 and p2 fixtures so they have at least 2 run script build phases. I've noticed the comparator outputs the build phases in random order (probably caused by the commonKeys: Set). Would be good to keep the order stable, sorting by name would suffice, we do the same with targets.

Great catch. Fixed in 6895eae. After having some experience with this, I almost wonder if a rethinking might be in order around the Build Phases entirely. It might be nice to be able to compare them directly with all the details being sort of "sub-comparators" that are called by the main comparison. That way you can get the results in a single spot and you can better take into consideration the order of the scripts when determining identity. Just a thought for the future!

3. Empty characters
   I've noticed you run into w swiftlint issues on CI. It's something we should probably document. We prefer to trim white characters. Xcode can do it automatically if you have it configured.
   

🤦🏼‍♂️ That was just embarrassing. I even saw the use of swiftformat and swiftlint in Development.md and forgot to run them. All fixed up in 5ed17ec.

4. Improving the output

After using this new functionality today, I'm wondering if perhaps the output could be improved by using an additional context value in the CompareResult to return more detailed information about the four properties that are lists of files: inputPaths, outputPaths, inputFileListPaths, and outputFileListPaths. I could use the onlyInFirst and onlyInSecond for them instead of DifferentValues. Is that something that makes sense?

That sounds like a great idea 👍 As you said, it does not need to be part of this PR, we can follow up on it.

Great!

Thanks again for your help. I also fixed up the copyright notices in c72261f.

[codecov]

Codecov Report

Merging #112 (356be5e) into main (d03725a) will increase coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head 356be5e differs from pull request most recent head e2fe490. Consider uploading reports for the commit e2fe490 to get more accurate results

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   96.54%   96.69%   +0.14%     
==========================================
  Files          49       50       +1     
  Lines        2636     2750     +114     
==========================================
+ Hits         2545     2659     +114     
  Misses         91       91              

Impacted Files	Coverage Δ
...es/XCDiffCore/Comparator/RunScriptComparator.swift	100.00% <100.00%> (ø)
Sources/XCDiffCore/ComparatorType.swift	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[marciniwanicki]

Many thanks @michaelmcguire for all the tweaks 👍, last small thing to resolve - DCO.

It would be best if you squash your commits into one, ideally with the description matching the PR description. We try to keep the history on the main branch linear. Once you have a single commit, you need to sign it, e.g. git commit --amend --signoff and force-push it.

[marciniwanicki]

Might be also good to update https://github.com/bloomberg/xcdiff/blob/main/Documentation/Comparators.md.

[michaelmcguire]

Many thanks @michaelmcguire for all the tweaks 👍, last small thing to resolve - DCO.

It would be best if you squash your commits into one, ideally with the description matching the PR description. We try to keep the history on the main branch linear. Once you have a single commit, you need to sign it, e.g. git commit --amend --signoff and force-push it.

@marciniwanicki All squashed and ready to go with an updated commit message that's been signed-off.

Might be also good to update https://github.com/bloomberg/xcdiff/blob/main/Documentation/Comparators.md.

Included that as well.

[marciniwanicki]

Nice work @michaelmcguire 🙌