Remove cross references #294 #297
Conversation
Generated by 🚫 Danger |
Codecov Report
@@ Coverage Diff @@
## master #297 +/- ##
=========================================
- Coverage 94.53% 94.5% -0.04%
=========================================
Files 99 99
Lines 5529 5639 +110
=========================================
+ Hits 5227 5329 +102
- Misses 302 310 +8
Continue to review full report at Codecov.
|
|
Hey @stupergenius, thanks for this 👍 I'm not going pretend that I understand the script. I'll have to look at it a bit more closely. It would be nice if you could integrate this with Danger like you mentioned. You asked for a little more context: Ultimately, we're trying to remove the dependency of any SwifterSwift extension on another SwifterSwift extension. It doesn't matter if both extensions are on the same type, in the same file, or in the same module. Each SwifterSwift extension should be able to stand alone if you were to copy & paste it into a project. |
|
Thanks @SD10. Am I on the right track with 73c1dc1? If so... this kind of use is quite prevalent throughout SwifterSwift so this effort might take some time... and it perhaps goes against the norm of DRY coding styles. I'm happy to continue along the same lines if this is how the project is intended to be structured, but without a linter capable of preventing such cross use (or a diligent code review team), this style might be difficult to maintain going forward. |
|
@stupergenius Yeah this is the right track. We don't need to do everything all at once if there are many instances where this structure occurs. I'd be willing to accept this PR -- but we have to take care of the build errors. As for not following the DRY principle, this is intentional. A common use case of SwifterSwift is using this project as a reference/collection of code snippets. We want to have each extension stand alone because it gets confusing when you want to copy & paste a single extension into your project -- then have to hunt down all the other dependencies. |
stupergenius
changed the title
|
So... I don't really know how to pass these coverage checks if the last commit doesn't push up the coverage enough. The lines that aren't covered are... very difficult to actually get covered (they're primarily graphics context methods that return nil in certain scenarios that don't happen in our SwifterSwift's use of them). They were also never actually covered, but by duplicating some methods it reduces coverage since there's now more overall code to cover. Suggestions? |
|
@stupergenius Don't worry about trivial code coverage deltas 😆 We just try to write tests to the best of our ability. The only check that matters is Travis passing 👍 I should set this up so that Travis is required and codecov is optional, thus making it more explicit |
|
Heh ok whew. Was about to get creative there. 😂 Otherwise this is ready to merge from my perspective. It still does not do the intra-file de-referencing, but it at least knocks out some cross-file references and provides context for later. |
There was a problem hiding this comment.
LGTM 👍 @LucianoPAlmeida any other input? Feel free to squash + merge if this is good for you
Checklist
Description
If I understood #294 correctly, then this should take care of cross-file references at least. I'm not sure of the issue with #286 since none of my findings include any UIKit extension references in the Foundation/Swift extension groups.
What this does not do, currently, is intra-file references to other extensions since this is a difficult one to automate (see below). Specifically, I'm wondering if examples like in NSAttributedString would need to be refactored (since extensions like
.boldeduse the helper methodapplying)? Or if just examples like NSURL should be refactored since it's a public extension method that proxies to another? Some guidance here would be appreciated.Side note: I created a small script that tries to compile each source file individually and emits any errors from the swift compiler to help automate this process. Perhaps this could be cleaned up and added to the Danger file with a nicer report (likely as a separate enhancement PR)?