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.94s vs 0.94s on main (0% slower)
📖 Linting Alamofire with this PR took 1.28s vs 1.28s on main (0% slower)
📖 Linting Brave with this PR took 7.47s vs 7.48s on main (0% faster)
📖 Linting DuckDuckGo with this PR took 5.02s vs 5.04s on main (0% faster)
📖 Linting Firefox with this PR took 10.99s vs 11.05s on main (0% faster)
📖 Linting Kickstarter with this PR took 10.1s vs 10.1s 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.55s vs 2.56s on main (0% faster)
📖 Linting Nimble with this PR took 0.78s vs 0.77s on main (1% slower)
📖 Linting PocketCasts with this PR took 8.62s vs 8.66s on main (0% faster)
📖 Linting Quick with this PR took 0.44s vs 0.43s on main (2% slower)
📖 Linting Realm with this PR took 4.71s vs 4.71s on main (0% slower)
📖 Linting Sourcery with this PR took 2.35s vs 2.36s on main (0% faster)
📖 Linting Swift with this PR took 4.61s vs 4.64s on main (0% faster)
📖 Linting VLC with this PR took 1.25s vs 1.25s on main (0% slower)
📖 Linting Wire with this PR took 18.4s vs 18.31s on main (0% slower)
📖 Linting WordPress with this PR took 11.78s vs 11.83s on main (0% faster)

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

@@ -336,5 +338,10 @@ internal struct MultilineParametersRuleExamples {
configuration: ["allows_single_line": false]),
Example("func ↓foo(param1: Int, param2: Bool, param3: [String]) { }",
configuration: ["allows_single_line": false]),
Example("""
func ↓foo(param1: Int,
param2: Bool, param3: [String]) { }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm trying to make sense of this new option. Why would it be helpful to allow this style given max_number_of_params = 3?

Copy link
Contributor Author

@kimdv kimdv Sep 5, 2024

Choose a reason for hiding this comment

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

Hmm. Maybe we should only allow x params etc, if they are on the same line?
Would that make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could in this case do the other way around?

So if it's set to 3, and the there is a newline then it will show a warning to put on a single line?
But maybe it would be better in that case to show a warning that all params should be on a newline instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The default style the rule enforces is that all parameters are on the same line or every parameter is on a separate line.

Optionally, with allows_single_line = false, all parameters need to be placed on their own line.

With the new option, you want to allow a specified number of parameters to be placed on a single line. Once the threshold is exceeded, all parameters are required to be placed on a separate line again. That's basically allows_single_line = true up to a number of parameters.

I think that enforcing a single line is not practicable, because parameters and their types could consume a lot of space and so placing them on separate line, even without the rule enforcing it, should be fine.

Now, how does max_number_of_parameters (maybe we need a better name) play nicely with allows_single_line = false? It's a contradiction. On the other hand, allows_single_line = true is redundant with max_number_of_parameters being configured with a reasonable number. We somehow need to align both, deprecate or reuse allows_single_line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay so I rearranged some code.

I still think that allows_single_line and max_number_of_parameters can live next to each other.
It's just, when allows_single_line is false then max_number_of_parameters is ignored. I think that is fine.

I added a test case, where allow single line is false but there is fewer params than max_number_of_parameters define.
Also I added one where there is 3 params on each line. Also within the allow number in max_number_of_parameters.

@kimdv kimdv force-pushed the kimdv/add-max-number-of-params-allowed branch 3 times, most recently from 00ab0c9 to 8fedd68 Compare September 17, 2024 17:50
@kimdv kimdv force-pushed the kimdv/add-max-number-of-params-allowed branch from 8fedd68 to 064dade Compare September 17, 2024 17:51
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