Cleans up some code #1517
Cleans up some code #1517
Conversation
|
|
||
| // MARK: - Network Protection | ||
|
|
||
| #if NETWORK_PROTECTION |
There was a problem hiding this comment.
Just wanted to remove all this logic from the app delegate, because it looks awful here.
I'm purposedly avoiding any changes to make the comparison of the old code and new code easy. We can do follow up PRs to make this better if we want.
mallexxx
left a comment
mallexxx
left a comment
There was a problem hiding this comment.
@diegoreymendez, I welcome the changes, but could you adjust the hardcoded dependencies since we‘re touching this.
Also it might be good adding couple of unit tests for it if you think it worths it
DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionAppEvents.swift
Show resolved
Hide resolved
| let loginItemsManager = NetworkProtectionLoginItemsManager() | ||
| let keychainStore = NetworkProtectionKeychainTokenStore() |
There was a problem hiding this comment.
since we‘re doing this, could you move these dependencies to the instance init with default values provided?
DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionAppEvents.swift
Show resolved
Hide resolved
DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionAppEvents.swift
Show resolved
Hide resolved
DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionAppEvents.swift
Show resolved
Hide resolved
|
Hey @mallexxx. I'm not aiming to modify and in fact I pretty much prefer to avoid modifying any of the code. The goal of this PR as stated in the scope section is to just take the code out of the app delegate. The reason I made this choice to not change anything was to avoid overcomplicating this, which is meant to be an atomic and quick change. Can we just evaluate if this works as before and move ahead please? |
|
@diegoreymendez, as you wish, just thought it'd be nice to add incremental changes that'll improve the code since technically it's the new files added 🙂 |
|
@mallexxx - Thank you very much! :) FWIW, I don't disagree with improving the code. It's just that for a PR where I'm only moving code around I'm usually trying to avoid changes on purpose to allow a quick and easy review. I'm happy if we want to keep pushing code improvements, and if we can do that in small and quick PRs I'll be happy to approve in a blast. For me it's not so much about avoiding changes, but about how much I believe small and contained PRs offer a lot of value. |
mallexxx
left a comment
mallexxx
left a comment
There was a problem hiding this comment.
LGTM with one naming note inline
|
Thanks @mallexxx, I appreciate it! |
diegoreymendez
deleted the
diego/prevent-netp-activation-without-authcode-step1
branch
# By Diego Rey Mendez (7) and others # Via Sam Symons (2) and others * develop: (26 commits) Improve Sync-related database cleaning logic (#1529) Update onboarding-related error states (#1504) Prevents launching our menu agent without an auth code. (#1516) Autofill UI letter icons (#1535) Cleans up some code (#1517) Revert "Autofill Letter Icons" (#1534) Adds remote pre-commit installer, which includes automatic fix for linter (#1369) Autofill Letter Icons (#1475) change context menu for mailto links (#1513) Updates the version to 1.53.1 Updated the embedded files for 1.53.1 Update the phased rollout tester to avoid caching the config (#1520) Require Duck Player scheme URL to be passed from YouTube Overlay User Script (#1519) Add pixels related to Duck Player usage (#1515) only allow error reloads on http(s) urls (#1523) Standardize TDS Loading Error handling (#1524) Move pixel sender logic into the main view controller (#1528) Update the phased rollout tester to avoid caching the config (#1520) Set version to 1.52.3. Move pixel sender logic into the main view controller (#1528) ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/AppDelegate/AppDelegate.swift # DuckDuckGo/Common/Localizables/UserText.swift # DuckDuckGo/Common/Utilities/UserDefaultsWrapper.swift
3 participants
Task/Issue URL: https://app.asana.com/0/0/1205318856585879/f
Description
Small code cleanup PR. There should be no logical changes.
Scope
This task is not meant to be a perfect cleanup, just moving some code to a separate file to make the app delegate less cluttered.
Steps to test this PR:
I recommend comparing the code with what we had to make sure nothing was changed.
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation