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

Disabled function_parameter_count rule SwiftLint #428

Merged
merged 3 commits into from
Jun 6, 2018

Conversation

gorbat-o
Copy link
Contributor

@gorbat-o gorbat-o commented May 9, 2018

  • I've added an entry in the CHANGELOG.md file to explain my changes and credit myself
  • Add the entry in the appropriate section (Bug Fix / New Feature / Breaking Change / Internal Change)
  • Add a period and 2 spaces at the end of your short entry description
  • Add links to your GitHub profile to credit yourself and to the related PR and issue after your description

This closes #427
Some keys generated by SwiftGen can have more then 5 parameters, it makes SwiftLint unhappy :'(

CHANGELOG.md Outdated
@@ -56,6 +56,9 @@
[David Jennes](https://github.com/djbe)
[#417](https://github.com/SwiftGen/SwiftGen/pull/417)
[#422](https://github.com/SwiftGen/SwiftGen/pull/422)
* Disabled a SwiftLint rule for function parameter count.
Copy link
Member

Choose a reason for hiding this comment

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

Missing 2 spaces at the end here.

CHANGELOG.md Outdated
@@ -56,6 +56,9 @@
[David Jennes](https://github.com/djbe)
[#417](https://github.com/SwiftGen/SwiftGen/pull/417)
[#422](https://github.com/SwiftGen/SwiftGen/pull/422)
* Disabled a SwiftLint rule for function parameter count.
[Oleg Gorbatchev](https://github.com/gorbat-o)
[#427](https://github.com/SwiftGen/SwiftGen/pull/417)
Copy link
Member

@djbe djbe May 9, 2018

Choose a reason for hiding this comment

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

Issue number and url are incorrect (we usually point to the PR, #428 in this case).

@@ -6,6 +6,7 @@ import Foundation

// swiftlint:disable superfluous_disable_command
// swiftlint:disable file_length
// swiftlint:disable function_parameter_count
Copy link
Member

Choose a reason for hiding this comment

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

I'd add this a bit lower, on line 41, with the other disables around the enum. I'd also re-enable it again after the enum. (same feedback for the other templates)

@@ -39,11 +38,13 @@ import Foundation
{% set enumName %}{{param.enumName|default:"L10n"}}{% endset %}
{{accessModifier}} enum {{enumName}} {
{% if tables.count > 1 %}
// swiftlint:disable function_parameter_count
Copy link
Collaborator

@AliSoftware AliSoftware May 9, 2018

Choose a reason for hiding this comment

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

Maybe I misunderstood what @djbe meant by suggesting you to move those next to the enum, but wouldn't it be more consistent to put that rule with the others just 4 lines above? i.e. add them to the list of already 3 disabled rules there?

// swiftlint:disable identifier_name line_length type_body_length function_parameter_count

_(of course same for when you re-enable the rule, I'd personally re-enable it in line 52 with the other rules)

I'm probably just nitpicking, but I personally feel easier to maintain having all swiftlint disabled rules grouped next to them when possible/appropriate 😉

Copy link
Member

Choose a reason for hiding this comment

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

Uh yeah, that's what I meant, apologies if that was unclear (I only quickly glanced at the line numbers).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree, I thought about that too,
But I strictly followed the demand 😅

@djbe djbe merged commit 6c7f07c into SwiftGen:master Jun 6, 2018
@djbe djbe added this to the Swiftgen 6.0 milestone Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SwiftLint Violation: Should have 5 parameters or less
3 participants