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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

1 Warning
⚠️ If this is a user-facing change, please include a CHANGELOG entry to credit yourself!
You can find it at CHANGELOG.md.
17 Messages
📖 Linting Aerial with this PR took 0.92s vs 0.96s on main (4% faster)
📖 Linting Alamofire with this PR took 1.26s vs 1.28s on main (1% faster)
📖 Linting Brave with this PR took 7.2s vs 7.21s on main (0% faster)
📖 Linting DuckDuckGo with this PR took 5.08s vs 5.1s on main (0% faster)
📖 Linting Firefox with this PR took 10.63s vs 10.64s on main (0% faster)
📖 Linting Kickstarter with this PR took 9.85s vs 9.81s on main (0% slower)
📖 Linting Moya with this PR took 0.53s vs 0.54s on main (1% faster)
📖 Linting NetNewsWire with this PR took 2.63s vs 2.63s on main (0% slower)
📖 Linting Nimble with this PR took 0.77s vs 0.77s on main (0% slower)
📖 Linting PocketCasts with this PR took 8.57s vs 8.47s on main (1% slower)
📖 Linting Quick with this PR took 0.44s vs 0.46s on main (4% faster)
📖 Linting Realm with this PR took 4.53s vs 4.5s on main (0% slower)
📖 Linting Sourcery with this PR took 2.3s vs 2.31s on main (0% faster)
📖 Linting Swift with this PR took 4.49s vs 4.48s on main (0% slower)
📖 Linting VLC with this PR took 1.24s vs 1.25s on main (0% faster)
📖 Linting Wire with this PR took 17.53s vs 17.5s on main (0% slower)
📖 Linting WordPress with this PR took 11.54s vs 11.51s on main (0% slower)

Here's an example of your CHANGELOG entry:

* Add max number if params allowed for `multiline_parameters`.  
  [kimdv](https://github.com/kimdv)
  [#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)

note: There are two invisible spaces after the entry's text.

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 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!

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