-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add a new non_optional_string_data_conversion
rule
#5264
Add a new non_optional_string_data_conversion
rule
#5264
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.
Thanks for bringing up this nice rule idea!
Please rebase your PR first as there have been quite some changes recently. Also, the OSS checks look broken. Other than that, I added a few comments.
Source/SwiftLintBuiltInRules/Rules/Lint/NonOptionalStringDataConversionRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/NonOptionalStringDataConversionRule.swift
Outdated
Show resolved
Hide resolved
Hey @SimplyDanny - the OSS violations being picked up now seem legitimate to me in the context of the rule that's been added. Not sure what you mean by broken here. Also, adopted your changes but tests are failing- is CICD having issues right now with swift / Xcode versions? |
There were previously no OSS changes at all. Something was broken in the reporting. It works now. I'll have a look into them later.
Please rebase your branch and get rid of all merge commits. There are thousands of changes in this PR at the moment. That can't be right. |
fececcf
to
8035573
Compare
Rebased. |
8035573
to
e5991aa
Compare
This rule was inspired by this issue: #5263.
I'm fairly new to
SwiftSyntax
so more efficient ideas/approaches are welcome 🙏🏻.