DNSChecker: simplified NetworkError initialization#2166
Conversation
4689061 to
36d1288
Compare
tonidero
left a comment
There was a problem hiding this comment.
A suggestion but looks good.
| return .dnsError(failedURL: failedURL, resolvedHost: host) | ||
| } | ||
|
|
||
| static func isBlockedURL(_ url: URL) -> Bool { |
There was a problem hiding this comment.
Should we make this private now? I believe it's not called outside this class other than it tests and we can test the public API for that.
There was a problem hiding this comment.
Oh yeah let me check.
There was a problem hiding this comment.
We have specific tests for this method, which I think is useful.
There was a problem hiding this comment.
Well, I try to avoid making things public only for tests. I think if it needs to be tested because the logic is too complex and the public API of the class does too many things, it should be extracted to a different class where it can be tested better. This doesn't seem to be that complex of a logic though, so I don't think it warrants moving to a different class.
That said, I don't think this should block the PR.
There was a problem hiding this comment.
Well, I try to avoid making things public only for tests. I think if it needs to be tested because the logic is too complex and the public API of the class does too many things, it should be extracted to a different class where it can be tested better.
💯
There was a problem hiding this comment.
I agree. The purpose of this PR was to avoid calling the DNS logic twice though.
`DNSChecker.errorWithBlockedHostFromError` already checks `isBlockedAPIError` when constructing this error, so we can simplify this to avoid calling `DNSChecker.resolvedHost` twice.
36d1288 to
7c20038
Compare
**This is an automatic release.** ### Bugfixes * `ErrorUtils.purchasesError(withUntypedError:)`: handle `PublicError`s (#2165) via NachoSoto (@NachoSoto) * Fixed race condition finishing `SK1` transactions (#2148) via NachoSoto (@NachoSoto) * `IntroEligibilityStatus`: added `CustomStringConvertible` implementation (#2182) via NachoSoto (@NachoSoto) * `BundleSandboxEnvironmentDetector`: fixed logic for `macOS` (#2176) via NachoSoto (@NachoSoto) * Fixed `AttributionFetcher.adServicesToken` hanging when called in simulator (#2157) via NachoSoto (@NachoSoto) ### Other Changes * `Purchase Tester`: added ability to purchase products directly with `StoreKit` (#2172) via NachoSoto (@NachoSoto) * `DNSChecker`: simplified `NetworkError` initialization (#2166) via NachoSoto (@NachoSoto) * `Purchases` initialization: refactor to avoid multiple concurrent instances in memory (#2180) via NachoSoto (@NachoSoto) * `Purchase Tester`: added button to clear messages on logger view (#2179) via NachoSoto (@NachoSoto) * `NetworkOperation`: added assertion to ensure that subclasses call completion (#2138) via NachoSoto (@NachoSoto) * `CacheableNetworkOperation`: avoid unnecessarily creating operations for cache hits (#2135) via NachoSoto (@NachoSoto) * `PurchaseTester`: fixed `macOS` support (#2175) via NachoSoto (@NachoSoto) * `IntroEligibilityCalculator`: added log including `AppleReceipt` (#2181) via NachoSoto (@NachoSoto) * `Purchase Tester`: fixed scene manifest (#2170) via NachoSoto (@NachoSoto) * `HTTPClientTests`: refactored to use `waitUntil` (#2168) via NachoSoto (@NachoSoto) * `Integration Tests`: split up tests in smaller files (#2158) via NachoSoto (@NachoSoto) * `StoreKitRequestFetcher`: removed unnecessary dispatch (#2152) via NachoSoto (@NachoSoto) * `Purchase Tester`: added companion `watchOS` app (#2140) via NachoSoto (@NachoSoto) * `StoreKit1Wrapper`: added warning if receiving too many updated transactions (#2117) via NachoSoto (@NachoSoto) * `StoreKitTestHelpers`: cleaned up unnecessary log (#2177) via NachoSoto (@NachoSoto) * `TrialOrIntroPriceEligibilityCheckerSK1Tests`: use `waitUntilValue` (#2173) via NachoSoto (@NachoSoto) * `DNSChecker`: added log with resolved host (#2167) via NachoSoto (@NachoSoto) * `MagicWeatherSwiftUI`: updated `README` to point to workspace (#2142) via NachoSoto (@NachoSoto) * `Purchase Tester`: fixed `.storekit` config file reference (#2171) via NachoSoto (@NachoSoto) * `Purchase Tester`: fixed error alerts (#2169) via NachoSoto (@NachoSoto) * `CI`: don't make releases until `release-checks` pass (#2162) via NachoSoto (@NachoSoto) * `Fastfile`: changed `match` to `readonly` (#2161) via NachoSoto (@NachoSoto)
DNSChecker.errorWithBlockedHostFromErroralready checksisBlockedAPIErrorwhen constructing this error, so we can simplify this to avoid callingDNSChecker.resolvedHosttwice.