-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add opt-in unneeded_throws_rethrows rule
#6069
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 opt-in unneeded_throws_rethrows rule
#6069
Conversation
Generated by 🚫 Danger |
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
|
@SimplyDanny Any thoughts on this? :) |
|
@tonyskansf It seems that some conflicts have arisen, would you mind resolving them? Thanks! |
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.
Looks very good already! You thought about many special cases already. The rule also makes sense. But adding it as opt-in is important as it can cause false positives in the context of protocol implementations (and maybe more occasions).
I've added a few comments. Please also check the findings reported by the OSS scan.
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRuleExamples.swift
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
|
You need to rebase onto |
3c154d1 to
bb93bfe
Compare
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
|
@SimplyDanny Please can you take a look again. Appreciate your time! Thank you |
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.
I'm back! However, a bit late. Sorry for that.
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/UnneededThrowsRuleExamples.swift
Show resolved
Hide resolved
5de437d to
7c1d566
Compare
7c1d566 to
b95ffe5
Compare
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 all the work, @tonyskansf! The PR is ready to land now. I really like the rule!
Add a new opt-in rule that detects unnecessary
throws, helping avoid misleading definitions and requirements for the caller to write unnecessary error handling.