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

Fix Swiftlint warnings #422

Merged
merged 6 commits into from
Oct 4, 2017

Conversation

Antondomashnev
Copy link
Collaborator

This one resolves #418. I've made couple of changes that I think will simplify development

  1. The rules for Templates are now in configuration file under Templates
  2. Swiftlint is used from CocoaPods, this allows to control the version of Swiftlint as with Homebrew it's not that easy.

Let me know if you have objections 😄

@SourceryBot
Copy link

SourceryBot commented Sep 25, 2017

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

Copy link
Collaborator

@Liquidsoul Liquidsoul left a comment

Choose a reason for hiding this comment

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

The change I request is about having swiftlint:disable on one line.

The vertical_whitespace issue I raised is entirely opened for discussion and will not stop me from approving this PR if the above change is done 👍

@@ -1,3 +1,6 @@
// swiftlint:disable vertical_whitespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure we should disable the vertical_whitespace rule.
In my opinion, we should strive to get generated code as clean as possible. We should not have multiple or inconsistent blank lines generated by the templates. So we should adapt the templates instead of disabling the rule for the entire file.

On the other hand, we cannot control everything and some rules should be disabled because we cannot do anything about them. The ones that come to my mind are:

  • identifier_name: identifier length or naming issues
  • type_name: types length or naming issues
  • file_length: we cannot control how long the output file will be because it depends on the user's code base

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Liquidsoul first of all, thanks for the review 👍
Regarding the vertical_whitespace I agree with you, I had weird output from Swiftlit which was clearly wrong so I put this disablement. I'll double check once again.

Regarding the proposals for other rules - I personally think that we should put it on the developer's shoulders. As there will always appear a new Swiftlint rule that will break the template code. But I'm also open to challenge it 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've checked once again and

{% for type in types.implementing.AutoCoding|class %}
{% if not type.supertype.implements.AutoCoding %}extension {{ type.name }}: NSCoding {}{% endif %}
    // sourcery:inline:{{ type.name }}.AutoCoding
        /// :nodoc:
        required {{ type.accessLevel }} init?(coder aDecoder: NSCoder) {
            {% for variable in type.storedVariables|!annotated:"skipCoding" %}{% if variable.typeName.name == "Bool" or variable.typeName.name == "Int" %}self.{{variable.name}} = aDecoder.decode(forKey: "{{variable.name}}"){% else %}{% if not variable.typeName.isOptional %}guard let {{variable.name}}: {{ variable.typeName.unwrappedTypeName }} = aDecoder.decode(forKey: "{{variable.name}}") else { NSException.raise(NSExceptionName.parseErrorException, format: "Key '%@' not found.", arguments: getVaList(["{{ variable.name }}"])); fatalError() }; self.{{variable.name}} = {{variable.name}}{% else %}self.{{variable.name}} = aDecoder.{% if variable.typeName.unwrappedTypeName == "Any" %}decodeObject{% else %}decode{% endif %}(forKey: "{{variable.name}}"){% endif %}{% endif %}
            {% endfor %}{% if type.supertype.implements.AutoCoding %}super.init(coder: aDecoder){% endif %}
        }

        /// :nodoc:
        {% if type.supertype.implements.AutoCoding %}override {% endif %}{{ type.accessLevel }} func encode(with aCoder: NSCoder) {
            {% if type.supertype.implements.AutoCoding %}super.encode(with: aCoder){% endif %}
            {% for variable in type.storedVariables|!annotated:"skipCoding" %}aCoder.encode(self.{{variable.name}}, forKey: "{{variable.name}}")
            {% endfor %}
        }
    // sourcery:end
    {% endfor %}

Generates unnecessary empty lines. It seems that there is an open PR which resolves the issues. So for now I will leave vertical_whitespace rule disabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The same is for the trailing_newline as it's also vertical whitespace.

@@ -1,3 +1,7 @@
// swiftlint:disable vertical_whitespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, you should put all the disabled rules in just one line.

@@ -1,3 +1,6 @@
// swiftlint:disable vertical_whitespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, you should put all the disabled rules in just one line.

@@ -1,6 +1,9 @@
// Generated using Sourcery 0.9.0 — https://github.com/krzysztofzablocki/Sourcery
// DO NOT EDIT

// swiftlint:disable vertical_whitespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, you should put all the disabled rules in just one line.

@@ -1,6 +1,9 @@
// Generated using Sourcery 0.9.0 — https://github.com/krzysztofzablocki/Sourcery
// DO NOT EDIT

// swiftlint:disable vertical_whitespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, you should put all the disabled rules in just one line.

@@ -1,3 +1,6 @@
// swiftlint:disable vertical_whitespace
// swiftlint:disable trailing_newline
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should put all the disabled rules in just one line:

// swiftlint:disable trailing_newline vertical_whitespace

@Antondomashnev Antondomashnev merged commit 8537989 into krzysztofzablocki:master Oct 4, 2017
@Antondomashnev Antondomashnev deleted the swiftlint branch October 4, 2017 19:35
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.

Update for new version of Swiftlint
3 participants