-
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
implements FunctionArgumentsSpacingRule #5337
base: main
Are you sure you want to change the base?
Conversation
@SimplyDanny Could you review this PR? I'm pretty new to SwiftSyntax, so any suggestions are welcome. |
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 taking this up!
Please add a changelog entry to let folks now about the new rule and to credit yourself.
We should also carefully review the reported OSS findings. Often, they disclose cases that we haven't thought of just yet.
Source/SwiftLintBuiltInRules/Rules/Lint/FunctionArgumentsSpacingRule.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftLintFrameworkTests/CyclomaticComplexityRuleTests.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/FunctionArgumentsSpacingRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/FunctionArgumentsSpacingRule.swift
Outdated
Show resolved
Hide resolved
} | ||
|
||
let leftParanTrailingTrivia = node.leftParen?.trailingTrivia | ||
if leftParanTrailingTrivia == Trivia.space { |
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.
What if there are multiple spaces? I don't think f( a )
would trigger the rule. Also comments are interesting, e.g. f( /* comment */ a /* other comment */ )
. Will it trigger?
return | ||
} | ||
|
||
let leftParanTrailingTrivia = node.leftParen?.trailingTrivia |
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's sometimes a bit surprising to which node SwiftSyntax attaches the whitespace. Therefore, you should also check the leading trivia of the first argument and the trailing trivia of the last argument.
e61b058
to
620741f
Compare
@SimplyDanny |
Please rebase your branch onto the current |
a6be49b
to
f884b3a
Compare
@SimplyDanny |
Merge is not the same as rebase. 😉 Danger fails because of merge commits which are undesirable. |
7083432
to
04e7abe
Compare
@SimplyDanny |
Tests/SwiftLintFrameworkTests/FunctionArgumentsSpacingRuleTests.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/FunctionArgumentsSpacingRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/FunctionArgumentsSpacingRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/FunctionArgumentsSpacingRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/FunctionArgumentsSpacingRule.swift
Outdated
Show resolved
Hide resolved
ab5ebba
to
aa032f5
Compare
…ngRule.swift Co-authored-by: Danny Mösch <[email protected]>
aa032f5
to
2f86326
Compare
f6688ee
to
dfbec83
Compare
@SimplyDanny |
closed #5224
implements
functions_arguments_spacing
rule to detects unnecessary spaces before the first argument of a function and unnecessary spaces in the last argument.nonTriggeringExamples:
triggeringExamples