-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Refactor FXIOS-10205 [Swiftlint] Enable implicitly_unwrapped_optional rule but keep it disabled for test files using nested Swiftlint configurations #24031
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great to me this is a really nice catch 🎉 🎉 . tagging also @OrlaM and @adudenamedruby to get extra eye on this modification since it impact a bit the project structure.
If this modification lands in i'll add it to our wiki.
@@ -15861,7 +15861,7 @@ | |||
); | |||
runOnlyForDeploymentPostprocessing = 0; | |||
shellPath = /bin/sh; | |||
shellScript = "if [[ \"$(uname -m)\" == arm64 ]]; then\n export PATH=\"/opt/homebrew/bin:$PATH\"\nfi\n\nMODIFIED_FILES=$(git diff --name-only --diff-filter=ACM | grep -e '\\.swift$')\n\nif which swiftlint > /dev/null; then\n for file in $MODIFIED_FILES; do\n swiftlint lint \"${SRCROOT}/../${file}\" --config \"${SRCROOT}/../.swiftlint.yml\"\n done\nelse\n echo \"warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint\" \nfi\n"; | |||
shellScript = "if [[ \"$(uname -m)\" == arm64 ]]; then\n export PATH=\"/opt/homebrew/bin:$PATH\"\nfi\n\nMODIFIED_FILES=$(git diff --name-only --diff-filter=ACM | grep -e '\\.swift$')\n\n# Location of the folder where the root swiftlint config is located.\n# Swiftlint commands will be ran from this location, allowing us to make use\n# of swiftlint's nested configurations to tweak rules for certain source\n# sets (e.g. test files).\n# Nested configuration can not work together with --config option, so we need\n# to run the swiftlint command from the directory containing the root\n# configuration file.\n# https://github.com/realm/SwiftLint#nested-configurations\nSWIFTLINT_ROOT=\"${SRCROOT}/../\"\n\nif which swiftlint > /dev/null; then\n for file in $MODIFIED_FILES; do\n ( cd ${SWIFTLINT_ROOT} && swiftlint lint \"${SRCROOT}/../${file}\" )\n done\nelse\n echo \"warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint\" \nfi\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small change the rest looks great 🎉 . i'd have:
// move out of the for loop
cd $SWIFTLINT_ROOT
for loop {}
I'd also reduce the comment a bit and add it directly to the cd $SWIFTLINT_ROOT saying that this instruction is needed here in order to have nested swiftlint configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done!
# nested configuration takes over. | ||
# https://github.com/realm/SwiftLint#nested-configurations. | ||
|
||
disabled_rules: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for the work on this issue 🙌 I'll let my colleagues do the final review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great to me. Thanks again for your contribution 🎉 🎉
Hi @FilippoZazzeroni, from the looks of the Bitrise output there are still some violations in However, it seems that the Bitrise Swiftlint phase script still uses the |
LGTM :). I'll let @FilippoZazzeroni push this through |
Hi @tonell-m sorry for the late response, i've been going deeper into your solution and i see some nits that i'd like to understand better. I'll look better into it tomorrow and let you know as soon as i have news. |
Hi @FilippoZazzeroni! I do agree with the points you're raising, personally (with the knowledge I have atm) I would suggest the following:
Let me know what you think when you'll have discussed it with your team ✌️ |
Thanks very much @tonell-m i like this idea, i'll come up with a proposal and let you know how we can proceed. |
Hi @tonell-m i've created this task with the results of the proposal. Let me know if you'd like to take on this. thanks again for all the good info, let me know if the proposal makes sense to you. |
Hi @FilippoZazzeroni! The task looks good, can you assign the issue to me please? I will look into it sometime this week 🙂 Only thing I think might be missing is updating the Bitrise configuration. Currently it seems to be using What bugs me is that looking at |
Hi @tonell-m yes of course. Can you please comment on github issue so i can assign you directly otherwise GitHub doesn't show your name on the assignee list. Anyway i agree with you for Bitrise i also read the code and theoretically it shouldn't use |
7feedec
to
cdbff29
Compare
let view: EngineView | ||
|
||
init(engine: Engine = WKEngine.factory(), | ||
sessionDependencies: EngineSessionDependencies? = nil) { | ||
init?(engine: Engine = WKEngine.factory(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is intended behavior, LGTM just tagging @lmarceau that worked on this implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, fast review! As I've mentioned below:
SampleBrowser doesn't compile anymore because RootViewController is missing a method from AddressToolbarDelegate
So I couldn't really test this fix, but it was the simplest workaround to get rid of the implicitly unwrapped property. Since engineSession
was implicitly unwrapped in BrowserViewController
it would have crashed anyway if engineProvider.session
was nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change works for me 👍
Hi @FilippoZazzeroni @lmarceau, now that #24219 has been merged I've updated this PR 😄
|
Thanks @tonell-m 🎉 , regarding Sample browser we can create a separate PR to fix it, let me know if you'd like to do it, i can create a ticket for you |
Generated by 🚫 Danger Swift against cdbff29 |
@FilippoZazzeroni sure I'm down for it! |
📜 Tickets
Jira ticket
Github issue
💡 Description
Now that all
implicitly_unwrapped_optional
violations in source files are resolved (still waiting on #23809 and #23880 to be merged), enable the rule in the Swiftlint configuration.Since most of the test files use implicitly unwrapped optional properties that are initialized in
setUp
and reset intearDown
, we would like not to enable this rule for test files. An approach is to use Swiftlint nested configurations by adding additional.swiftlint.yml
files that can alter the global configuration for subfolders (in this case, disableimplicitly_unwrapped_optional
forTests
folders).Nested configurations cannot be used together with the
--config
option to point to the Swiftlint config file, so the workaround is to navigate to the directory containing the config before runningswiftlint
in the build phase.📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120
)