Add FXIOS-8821 [SwiftLint] for_where #19889
Conversation
|
This pull request has conflicts when rebasing. Could you fix it @cavaevinci? 馃檹 |
|
@thatswinnie , fixed merge conflicts |
Generated by 馃毇 Danger Swift against 215f226 |
| continue domainSearch | ||
| } | ||
| for rule in rules { | ||
| guard let _ = rule.regex.firstMatch(in: resourceString, options: .anchored, range: resourceRange) else { |
There was a problem hiding this comment.
Let's add the comment back in:
// First, test the top-level filters to see if this URL might be blocked.
| guard let _ = rule.regex.firstMatch(in: resourceString, options: .anchored, range: resourceRange) else { | ||
| continue | ||
| } | ||
| let hasDomainException = rule.domainExceptions?.contains { domainRegex in |
There was a problem hiding this comment.
Let's add the comment back here as well:
// Check the domain exceptions. If a domain exception matches, this filter does not apply.
| guard !hasDomainException else { | ||
| continue | ||
| } | ||
| if let baseDomain = url.baseDomain, !permittedDomains.isEmpty { |
There was a problem hiding this comment.
And this comment should be added back too:
// Check the whitelist.
| return nil | ||
| } | ||
| } | ||
| guard !isPermittedDomain else { |
There was a problem hiding this comment.
If it is a permitted domain we should return nil instead of continuing.
| weakDelegate.delegate = delegate | ||
| return | ||
| } | ||
| if let availableSlot = delegates.first(where: { $0.delegate == nil }) { |
There was a problem hiding this comment.
Please add the comment back:
// Reuse any existing slots that have been deallocated.
| if let lastIndex = currentStack.firstIndex(where: { !$0.isCurrentSearch }) { | ||
| currentStack.removeSubrange(lastIndex + 1..<currentStack.count) |
There was a problem hiding this comment.
This change doesn't look correct. The stack should only be cleaned up if the lastSearch is not the current search.
| if let lastIndex = currentStack.firstIndex(where: { !$0.isCurrentSearch }) { | |
| currentStack.removeSubrange(lastIndex + 1..<currentStack.count) | |
| // Check whether the lastSearch is the current search. If not, remove subsequent searches | |
| if let lastSearch = currentStack.last, | |
| !lastSearch.isCurrentSearch, | |
| let lastIndex = currentStack.firstIndex(where: { !$0.isCurrentSearch }) { | |
| currentStack.removeSubrange(lastIndex + 1..<currentStack.count) |
There was a problem hiding this comment.
Indeed, the change might not be correct. We want go and look for searches to be removed only if the last search is not the current search. Then we look for the first search that is current search and remove everything after it (not including it). After that we mark everything as not being current search and only afteeer that we add the new search and mark it accordingly. Ugh that was a lot. A similar implementation the might be more clear can be:
static func pushSearchToStack(with searchedText: String) {
// Check if the stack is empty and directly append if it is
guard !currentStack.isEmpty else {
currentStack.append(textSearched(text: searchedText, isCurrentSearch: true))
return
}
// Find the last `current search` and truncate the stack beyond this point
if let lastIndex = currentStack.lastIndex(where: { $0.isCurrentSearch }) {
currentStack = Array(currentStack.prefix(upTo: lastIndex + 1))
}
// Mark all as not current in a single pass and append new search
currentStack = currentStack.map { textSearched(text: $0.text, isCurrentSearch: false) }
currentStack.append(textSearched(text: searchedText, isCurrentSearch: true))
}To show an example we start with
0: "apple" (isCurrentSearch = false)
1: "banana" (isCurrentSearch = false)
2: "cherry" (isCurrentSearch = true)
3: "date" (isCurrentSearch = false)
Then we truncate after index 2
0: "apple" (isCurrentSearch = false)
1: "banana" (isCurrentSearch = false)
2: "cherry" (isCurrentSearch = true)
We set everything to false
0: "apple" (isCurrentSearch = false)
1: "banana" (isCurrentSearch = false)
2: "cherry" (isCurrentSearch = false)
Then we add the search
0: "apple" (isCurrentSearch = false)
1: "banana" (isCurrentSearch = false)
2: "cherry" (isCurrentSearch = false)
3: "elderberry" (isCurrentSearch = true)
| let hasDomainException = rule.domainExceptions?.contains { domainRegex in | ||
| domainRegex.firstMatch(in: resourceString, options: [], range: resourceRange) != nil | ||
| } ?? false | ||
| guard !hasDomainException else { |
There was a problem hiding this comment.
nit: Although guarding can make the early exits more readable in this case the inversion makes me a bit confused.
I would use if hasDomainException { continue } implying that if we indeed find an exception we skip to the next rule.
|
This pull request has conflicts when rebasing. Could you fix it @cavaevinci? 馃檹 |
|
This PR has been automatically marked as stale. Please leave any comment to keep this PR opened. It will be closed automatically if no further update occurs in the next 7 days. Thank you for your contributions! |
github-actions
bot
added
the
stale
馃摐 Tickets
Jira ticket
Github issue
馃挕 Description
Implemented for_where swiftlint rule
馃摑 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)