Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

Update code style to common Swift style #125

Merged
Hugo Tunius (k0nserv) merged 7 commits into from
Apr 17, 2017
Merged

Update code style to common Swift style #125

Hugo Tunius (k0nserv) merged 7 commits into from
Apr 17, 2017

Conversation

@moogle19
Copy link
Contributor

@moogle19 Kevin (moogle19) commented Apr 12, 2017

Hi,
I updated some of the variable declarations with spaces after the colons and also omitted self's where possible (as recommended).

@k0nserv
Copy link
Contributor

Hugo Tunius (k0nserv) commented Apr 12, 2017 •
edited
Loading

This is great 👍 I've been thinking about swiftlint recently and this looks like a good step in that direction. Feel free to add it to this PR if you feel inclined, otherwise I'll do it on the back of your work.

@k0nserv
Copy link
Contributor

Hugo Tunius (k0nserv) commented Apr 12, 2017

Also please credit yourself in CHANGELOG.md under the master section

@moogle19
Add swiftlint
09d1056 
Added swiftlint build phase and fixed errors and warnings generated by
swiftlint
@moogle19
Revert ae49746
6dc58ec
@moogle19
Copy link
Contributor Author

Kevin (moogle19) commented Apr 13, 2017

Hugo Tunius (@k0nserv)
I added swiftlint as a Build Phase (not sure if should run automatically on build or manually?) and fixed the errors and warnings generated by swiftlint.

I used the default swiftlint settings (not sure if something specific should be enabled/disabled).

To fix the line length warnings (120 characters) I used this style of indentation.

addErrorButton?.setTitle(NSLocalizedString("Add error",
                                           tableName: "SkyFloatingLabelTextField",
                                           comment: "add error button title"), for: .normal)

I hope this is okay or is there a preferable style?

Let me know when something is wrong or needs change.

(There also was a commit I did not intend to push which I reverted afterwards)

@k0nserv
Copy link
Contributor

Hugo Tunius (k0nserv) commented Apr 14, 2017 •
edited
Loading

I think I prefer this style

addErrorButton?.setTitle(
    NSLocalizedString("Add error",
    tableName: "SkyFloatingLabelTextField",
    comment: "add error button title"), for: .normal
)

Emil Bogren (@bogren) opinions?

@bogren
Copy link
Contributor

Emil Bogren (bogren) commented Apr 14, 2017 •
edited
Loading

Or like this? 😃

addErrorButton?.setTitle(NSLocalizedString("Add error",
                                           tableName: "SkyFloatingLabelTextField",
                                           comment: "add error button title"),
                         for: .normal
)

I think I prefer Hugo Tunius (@k0nserv) suggestion but put the next parameter below as well :)

addErrorButton?.setTitle(
    NSLocalizedString("Add error",
                      tableName: "SkyFloatingLabelTextField",
                      comment: "add error button title"),
    for: .normal
)

@k0nserv
Copy link
Contributor

Hugo Tunius (k0nserv) commented Apr 14, 2017

This is how I would write it

addErrorButton?.setTitle(
    NSLocalizedString(
        "Add error",
        tableName: "SkyFloatingLabelTextField",
        comment: "add error button title"
    ),
    for: .normal
)

k0nserv
k0nserv approved these changes Apr 17, 2017
View reviewed changes
Copy link
Contributor

@k0nserv Hugo Tunius (k0nserv) left a comment

Choose a reason for hiding this comment

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

Awesome, looks great :)

@k0nserv Hugo Tunius (k0nserv) merged commit e48d531 into Skyscanner:master Apr 17, 2017
@k0nserv
Copy link
Contributor

Hugo Tunius (k0nserv) commented Apr 17, 2017

Thanks a bunch for the PR Kevin (@moogle19)

@moogle19 Kevin (moogle19) deleted the style_update branch April 17, 2017 10:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@k0nserv Hugo Tunius (k0nserv) k0nserv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@moogle19 @k0nserv @bogren