Skip to content
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 SwiftLint rule to prevent multiple variable declarations on one line #5990

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

AhmedZaghloul19
Copy link

for the issue:
Rule Request: Avoid multiple statements on a single line separated with semicolons #5829

@SwiftLintBot
Copy link

SwiftLintBot commented Feb 5, 2025

18 Messages
📖 Building this branch resulted in a binary size of 29548.49 KiB vs 29537.85 KiB when built on main (0% larger).
📖 Linting Aerial with this PR took 0.95 s vs 0.94 s on main (1% slower).
📖 Linting Alamofire with this PR took 1.27 s vs 1.28 s on main (0% faster).
📖 Linting Brave with this PR took 8.55 s vs 8.53 s on main (0% slower).
📖 Linting DuckDuckGo with this PR took 5.78 s vs 5.78 s on main (0% slower).
📖 Linting Firefox with this PR took 11.7 s vs 11.71 s on main (0% faster).
📖 Linting Kickstarter with this PR took 9.86 s vs 9.91 s on main (0% faster).
📖 Linting Moya with this PR took 0.52 s vs 0.52 s on main (0% slower).
📖 Linting NetNewsWire with this PR took 2.88 s vs 2.88 s on main (0% slower).
📖 Linting Nimble with this PR took 0.78 s vs 0.78 s on main (0% slower).
📖 Linting PocketCasts with this PR took 8.72 s vs 8.75 s on main (0% faster).
📖 Linting Quick with this PR took 0.45 s vs 0.43 s on main (4% slower).
📖 Linting Realm with this PR took 4.54 s vs 4.53 s on main (0% slower).
📖 Linting Sourcery with this PR took 2.35 s vs 2.32 s on main (1% slower).
📖 Linting Swift with this PR took 4.69 s vs 4.68 s on main (0% slower).
📖 Linting VLC with this PR took 1.31 s vs 1.31 s on main (0% slower).
📖 Linting Wire with this PR took 18.55 s vs 18.5 s on main (0% slower).
📖 Linting WordPress with this PR took 11.36 s vs 11.38 s on main (0% faster).

Generated by 🚫 Danger

@SimplyDanny
Copy link
Collaborator

Thank you for the contribution! Here are some general remarks:

  • The rule is supposed to trigger on multiple statements on a single line. That is, it is not only about variable declarations, but all kinds of statements like let a = 1; return a or if b { return }; let a = 1.
  • Refer to TrailingSemicolonRule to get an idea of how this could be implemented.
  • Run make sourcery to get the order of tests and the rule list in the right order.
  • 19980 warnings are surely too many. I expect a handful or warnings or even none.
  • The explicit test MultipleVariableDeclarationRuleTests isn't required. Rather add more triggering and non-triggering examples to the rule's description.

@AhmedZaghloul19
Copy link
Author

Thank you @SimplyDanny, yes it was unnecessary to have unit tests for it, I think now it'll work properly.
and about this pipeline error, I really don't know why!, if you can help me fixing it it would be great.
it was successful before solving the merge conflict

@SimplyDanny
Copy link
Collaborator

and about this pipeline error, I really don't know why!, if you can help me fixing it it would be great.

Danger doesn't like merge commits, that's all. You need to rebase your branch onto main instead.

@AhmedZaghloul19
Copy link
Author

@SimplyDanny I'm trying to rebase on main but I'm afraid of messing everything up actually, as there is conflicts while rebasing and I'm afraid of not including your changes 😬

@SimplyDanny
Copy link
Collaborator

@SimplyDanny I'm trying to rebase on main but I'm afraid of messing everything up actually, as there is conflicts while rebasing and I'm afraid of not including your changes 😬

You definitely messed something up. You can try to rebase again and throw away all commits that are not yours or duplicates of commits you have created previously. There shouldn't be any conflicts apart from maybe CHANGELOG.md, which would be easy to resolve.

@AhmedZaghloul19 AhmedZaghloul19 force-pushed the multiple-variable-declaration-rule branch 3 times, most recently from 25b3805 to 7f6556e Compare February 19, 2025 22:18
@AhmedZaghloul19 AhmedZaghloul19 force-pushed the multiple-variable-declaration-rule branch from 7f6556e to 92b6a81 Compare February 19, 2025 22:23
@AhmedZaghloul19
Copy link
Author

@SimplyDanny I'm trying to rebase on main but I'm afraid of messing everything up actually, as there is conflicts while rebasing and I'm afraid of not including your changes 😬

You definitely messed something up. You can try to rebase again and throw away all commits that are not yours or duplicates of commits you have created previously. There shouldn't be any conflicts apart from maybe CHANGELOG.md, which would be easy to resolve.

@SimplyDanny the git now is fixed and everything is ready 👍🏼

@SimplyDanny
Copy link
Collaborator

@SimplyDanny the git now is fixed and everything is ready 👍🏼

Commits look okay now. But there's still a few things to consider. Did you read my other comment carefully?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants