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

Removed SPMUtility from XCDiffCore #27

Merged

Conversation

tbrachkov
Copy link
Contributor

@tbrachkov tbrachkov commented Nov 18, 2019

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

Describe your changes
Removed the SPMUtility from XCDiffCore. Added replacement for smp_split and relative path to base path.

Testing performed
Added unit test for the new helpers.

Additional context
By removing SPMUtility from XCDiffCore now we can enable CocoaPods for XCDiffCore.

Signed-off-by: Todor Brachkov tbrachkov@gmail.com

…nd relative path to base path.

Signed-off-by: Todor Brachkov <tbrachkov@gmail.com>
Signed-off-by: Todor Brachkov <tbrachkov@gmail.com>
Signed-off-by: Todor Brachkov <tbrachkov@gmail.com>
Signed-off-by: Todor Brachkov <tbrachkov@gmail.com>
Copy link
Contributor

@marciniwanicki marciniwanicki left a comment

Choose a reason for hiding this comment

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

Many thanks @tbrachkov, the PR looks great! I am super excited - dropping a dependency is always an achievement! Left few very minor comments / questions.

Tests/XCDiffCoreTests/Library/StringExtensionsTests.swift Outdated Show resolved Hide resolved
Tests/XCDiffCoreTests/Library/StringExtensionsTests.swift Outdated Show resolved Hide resolved
delimiter = "::"
}

func test_split_empty_string() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have very informal, but commonly used pattern for naming the tests:
test<method_name>_[when]<condition>_[<expected_result>]

  • when is optional
  • expected result is also optional

i.e.
testSplit_whenStringNotContainDelimiter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the style guide, renamed the test functions.

equal(testString.split(around: delimiter), ("foo", "bar"))
}

func equal(_ lhs: (String, String?), _ rhs: (String, String?), file: StaticString = #file, line: UInt = #line) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice matcher! Maybe we can add private and rename to highlight it asserts? i.e. assertEqual ?
We can also replace lhs by actual and rhs by expected.

and lastly.. we usually add

// MARK: - Private

above the private section :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what could be the implication of those methods not being private? I have made all of the helpers private and marked as private.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's very minor. We usually try to limit the visibility of types, properties, methods to bare minimum in both production and test code. In general, it makes it easier to reason about the code (and hopefully helps the compiler a little bit as well) , but agree, in this specific case it's more for consistency than anything else :)

Tests/XCDiffCoreTests/Library/URLExtensionsTests.swift Outdated Show resolved Hide resolved
Tests/XCDiffCoreTests/Library/URLExtensionsTests.swift Outdated Show resolved Hide resolved
Tests/XCDiffCoreTests/Library/URLExtensionsTests.swift Outdated Show resolved Hide resolved
• Updated helper functions in tests to be marked as private.
• Renamed test functions to match the style guide.
• Extended test for edge cases on the relative path tests.

Signed-off-by: Todor Brachkov <tbrachkov@gmail.com>
Copy link
Contributor

@marciniwanicki marciniwanicki left a comment

Choose a reason for hiding this comment

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

Thank you @tbrachkov for all the changes!

@marciniwanicki marciniwanicki merged commit f85be01 into bloomberg:master Nov 20, 2019
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.

None yet

2 participants