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 max number if params allowed for multiline_parameters #5781

Merged
merged 3 commits into from
Nov 24, 2024

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Sep 2, 2024

This PR will configuration option for multiline_parameters so it's allowed to have like two params on the same line.

@kimdv kimdv force-pushed the kimdv/add-max-number-of-params-allowed branch from e7d445b to 48c7fb4 Compare September 2, 2024 14:45
@SwiftLintBot
Copy link

SwiftLintBot commented Sep 2, 2024

17 Messages
📖 Linting Aerial with this PR took 0.92s vs 0.93s on main (1% faster)
📖 Linting Alamofire with this PR took 1.26s vs 1.27s on main (0% faster)
📖 Linting Brave with this PR took 7.2s vs 7.21s on main (0% faster)
📖 Linting DuckDuckGo with this PR took 5.12s vs 5.16s on main (0% faster)
📖 Linting Firefox with this PR took 10.71s vs 10.73s on main (0% faster)
📖 Linting Kickstarter with this PR took 9.89s vs 10.0s on main (1% faster)
📖 Linting Moya with this PR took 0.53s vs 0.54s on main (1% faster)
📖 Linting NetNewsWire with this PR took 2.9s vs 2.92s on main (0% faster)
📖 Linting Nimble with this PR took 0.78s vs 0.79s on main (1% faster)
📖 Linting PocketCasts with this PR took 8.5s vs 8.53s on main (0% faster)
📖 Linting Quick with this PR took 0.46s vs 0.47s on main (2% faster)
📖 Linting Realm with this PR took 4.48s vs 4.49s on main (0% faster)
📖 Linting Sourcery with this PR took 2.3s vs 2.32s on main (0% faster)
📖 Linting Swift with this PR took 4.5s vs 4.51s on main (0% faster)
📖 Linting VLC with this PR took 1.25s vs 1.26s on main (0% faster)
📖 Linting Wire with this PR took 17.78s vs 17.75s on main (0% slower)
📖 Linting WordPress with this PR took 11.35s vs 11.35s on main (0% slower)

Generated by 🚫 Danger

@kimdv kimdv force-pushed the kimdv/add-max-number-of-params-allowed branch 4 times, most recently from 8fedd68 to 064dade Compare September 17, 2024 17:51
@kimdv kimdv requested a review from SimplyDanny September 17, 2024 17:51
@kimdv kimdv force-pushed the kimdv/add-max-number-of-params-allowed branch 3 times, most recently from 98e3679 to d3d8886 Compare October 14, 2024 12:22
@kimdv kimdv force-pushed the kimdv/add-max-number-of-params-allowed branch 3 times, most recently from f724f3a to c858669 Compare October 22, 2024 19:18
Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism seems to work. 👍

Let's have a separate test verifying that these issues are thrown. You can intercept the console output with the help of Issue.captureConsole { ... }.

@kimdv kimdv force-pushed the kimdv/add-max-number-of-params-allowed branch from c858669 to 5cb3560 Compare October 28, 2024 20:12
@kimdv
Copy link
Contributor Author

kimdv commented Oct 28, 2024

Issue.captureConsole

I've tried to make some tests, but I can't seem to get it work as you suggest.
I think it's because it throws the error?

I get this

XCTAssertEqual failed: threw error "invalidConfiguration(ruleID: "multiline_parameters", message: Optional("Option \'max_number_of_single_line_parameters\' should be >= 1."))"

I'm considering to do something like

XCTAssertThrowsError(try config.apply(configuration: ["max_number_of_single_line_parameters": 2, "allows_single_line": false])) { error in
            
        }

Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good. Thanks for the updates and for adding tests!

@SimplyDanny SimplyDanny force-pushed the kimdv/add-max-number-of-params-allowed branch from 5cb3560 to ccc7760 Compare November 24, 2024 13:42
@SimplyDanny SimplyDanny enabled auto-merge (squash) November 24, 2024 13:50
@SimplyDanny
Copy link
Collaborator

Issue.captureConsole

I've tried to make some tests, but I can't seem to get it work as you suggest. I think it's because it throws the error?

I fixed that by reporting a warning only instead of throwing an error. The rule still behaves reasonably even with confusing option combinations.

@SimplyDanny SimplyDanny merged commit 2a723d0 into realm:main Nov 24, 2024
14 checks passed
@kimdv kimdv deleted the kimdv/add-max-number-of-params-allowed branch November 25, 2024 08:33
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.

3 participants