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

Allow displaying only enabled rules in RulesCommand #1275

Closed
wants to merge 18 commits into from

Conversation

natanrolnik
Copy link
Contributor

@natanrolnik natanrolnik commented Feb 2, 2017

See #1270.
I've also updated the Commandant submodule to include the fix for Switches included in this PR.

@SwiftLintBot
Copy link

SwiftLintBot commented Feb 2, 2017

3 Warnings
⚠️ This PR may need tests.
⚠️ This PR introduced a violation in ios-oss: /Kickstarter-iOS/Views/Cells/RewardCell.swift#L57:31: warning: Large Tuple Violation: Tuples should have at most 2 members. (large_tuple)
⚠️ This PR introduced a violation in ios-oss: /Library/Paginate.swift#L51:3: error: Large Tuple Violation: Tuples should have at most 3 members. (large_tuple)
13 Messages
📖 Linting WordPress-iOS with this PR took 17.8s vs 20.53s on master (13% faster)
📖 Linting Alamofire with this PR took 3.66s vs 4.79s on master (23% faster)
📖 Linting swift with this PR took 13.25s vs 17.46s on master (24% faster)
📖 Linting Aerial with this PR took 0.49s vs 0.68s on master (27% faster)
📖 Linting SourceKitten with this PR took 1.44s vs 1.91s on master (24% faster)
📖 Linting Sourcery with this PR took 2.82s vs 3.51s on master (19% faster)
📖 This PR fixed a violation in ios-oss: /Library/Paginate.swift#L51:3: warning: Large Tuple Violation: Tuples should have at most 2 members. (large_tuple)
📖 Linting Moya with this PR took 0.58s vs 0.59s on master (1% faster)
📖 Linting firefox-ios with this PR took 25.07s vs 27.76s on master (9% faster)
📖 Linting Nimble with this PR took 2.59s vs 3.16s on master (18% faster)
📖 Linting Quick with this PR took 0.81s vs 0.93s on master (12% faster)
📖 Linting realm-cocoa with this PR took 4.29s vs 4.9s on master (12% faster)
📖 Linting ios-oss with this PR took 21.28s vs 23.34s on master (8% faster)

Generated by 🚫 danger

@natanrolnik
Copy link
Contributor Author

natanrolnik commented Feb 2, 2017

Currently, swiftlint rules --enabled doesn't work the same way as swiftlint rules -e due to a bug in Commandant. I've submitted a PR to fix it there.

CHANGELOG.md Outdated
@@ -23,6 +23,10 @@
[Marcelo Fabri](https://github.com/marcelofabri)
[#1061](https://github.com/realm/SwiftLint/issues/1061)

* Allow displaying only enabled rules in `RulesCommand`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is missing two trailing whitespaces

return .success()
}

func ruleList(for options: RulesOptions, configuration: Configuration) -> RuleList {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be private

return { ruleID in
self.init(ruleID: (ruleID.isEmpty ? nil : ruleID), configurationFile: configurationFile)
}
// swiftlint:disable line_length
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use swiftlint:disable:next line_length instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, will change that

static func evaluate(_ mode: CommandMode) -> Result<RulesOptions, CommandantError<CommandantError<()>>> {
return create
<*> mode <| configOption
<*> mode <| Argument(defaultValue: "",
usage: "the rule identifier to display description for")
<*> mode <| Switch(flag: Character("e"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

can't you just use "e"?. I expected this to work because of StringLiteralConvertible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try

@natanrolnik
Copy link
Contributor Author

@marcelofabri fixed all your comments. Also, why are the trailing whitespaces needed at the end of each issue line in the Changelog?

@natanrolnik natanrolnik force-pushed the filter-enabled-rules branch 3 times, most recently from 6ef8ac2 to c81bcc1 Compare February 5, 2017 15:03
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@3f6d528). Click here to learn what that means.

@@            Coverage Diff            @@
##             master    #1275   +/-   ##
=========================================
  Coverage          ?   81.75%           
=========================================
  Files             ?      168           
  Lines             ?     8363           
  Branches          ?        0           
=========================================
  Hits              ?     6837           
  Misses            ?     1526           
  Partials          ?        0
Impacted Files Coverage Δ
Source/swiftlint/Commands/RulesCommand.swift 0% <ø> (ø)
...rce/SwiftLintFramework/Models/MasterRuleList.swift 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f6d528...c81bcc1. Read the comment docs.

Copy link
Collaborator

@norio-nomura norio-nomura left a comment

Choose a reason for hiding this comment

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

Could you please update Cartfile* to reflect changes of Carthage/Checkouts/Commandant?
The steps would be:

  1. Edit Cartfile.private to use master of Commandant
  2. carthage update Commandant --no-build

@natanrolnik
Copy link
Contributor Author

@norio-nomura @marcelofabri I've made the changes you requested.
I had a hard time rebasing this branch/PR, so I've reopened as #1293

@natanrolnik natanrolnik closed this Feb 8, 2017
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.

7 participants