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

Add dependencies comparator #44

Conversation

marciniwanicki
Copy link
Contributor

Resolves: #41

Describe your changes
Added new comparator to compare "Dependencies" build phases.

Usage:

xcdiff -g dependencies

Testing performed

  • Compare the same project to itself

    • Run xcdiff -p1 Fixtures/ios_project_1/Project.xcodeproj -p2 Fixtures/ios_project_1/Project.xcodeproj -g dependencies
    • Verify no differences are flagged
  • Compare two different projects

    • Run xcdiff -p1 Fixtures/ios_project_1/Project.xcodeproj -p2 Fixtures/ios_project_2/Project.xcodeproj -g dependencies
    • Verify dependencies differences are flagged correctly

@codecov
Copy link

codecov bot commented Feb 2, 2020

Codecov Report

Merging #44 into master will increase coverage by 0.05%.
The diff coverage is 97.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   96.12%   96.17%   +0.05%     
==========================================
  Files          43       44       +1     
  Lines        1598     1672      +74     
==========================================
+ Hits         1536     1608      +72     
- Misses         62       64       +2     
Impacted Files Coverage Δ
...XCDiffCore/Comparator/DependenciesComparator.swift 97.14% <97.14%> (ø)
...Core/Comparator/LinkedDependenciesComparator.swift 92.00% <100.00%> (ø)
Sources/XCDiffCore/ComparatorType.swift 100.00% <100.00%> (ø)
Sources/XCDiffCore/Library/TargetsHelper.swift 93.54% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa9528d...3bc8d9a. Read the comment docs.

Copy link
Contributor

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Nice work 👍

We need to revisit how we extract the dependency names to make it cover more cases :)

// MARK: - Private

private func compare(_ first: PBXTarget, _ second: PBXTarget) throws -> [CompareResult] {
let firstNames = first.dependencies.map { $0.name ?? $0.target!.name }.toSet()
Copy link
Contributor

Choose a reason for hiding this comment

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

This will sadly crash in the event a project has a swift package dependency ... in that scenario there is no name, nor target! in fact looking at PBXTargetDependency there's even another type targetProxy!

The others are a bit easier:

   private func resolveDependencyDescription(from dependency: PBXTargetDependency) -> String? {
        let nameCandidates = [
            dependency.name,
            dependency.target?.name,
            dependency.product?.productName
        ]

        return nameCandidates.compactMap { $0 }.first
    }

For the target proxy we may have to construct some form or description based on the reference and maybe annotate it's a remote target or something along those lines.

@marciniwanicki marciniwanicki force-pushed the feature/dependencies-comparator branch 5 times, most recently from ad2a4ab to ee66600 Compare May 30, 2020 13:05
@kwridan
Copy link
Contributor

kwridan commented Jul 2, 2020

@marciniwanicki mind a rebase on master?

Signed-off-by: Marcin Iwanicki <[email protected]>
Signed-off-by: Marcin Iwanicki <[email protected]>
Signed-off-by: Marcin Iwanicki <[email protected]>
Signed-off-by: Marcin Iwanicki <[email protected]>
@marciniwanicki marciniwanicki force-pushed the feature/dependencies-comparator branch from ee66600 to 3bc8d9a Compare July 2, 2020 09:27
Copy link
Contributor

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Nice work @marciniwanicki 👍

Tested it out with local swift packages

A minor suggestion for next time possibly a space around the = or maybe event have it as :

from:

❌ DEPENDENCIES > "Project" target

⚠️  Only in second (3):

  • (product_name=MyLibrary, context=none)
  • (target=MismatchingLibrary)
  • (target=NewFramework)

to:

❌ DEPENDENCIES > "Project" target

⚠️  Only in second (3):

  • (product_name: MyLibrary, context: none)
  • (target: MismatchingLibrary)
  • (target: NewFramework)

Happy to do it as a follow up myself if you think it looks nicer.

@marciniwanicki marciniwanicki merged commit 9ee0650 into bloomberg:master Jul 7, 2020
@marciniwanicki marciniwanicki deleted the feature/dependencies-comparator branch July 7, 2020 07:32
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.

Extend dependencies comparator to include target dependencies
2 participants