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 parsing of rule disable command comments containing a URL #2985

Merged

Conversation

svenmuennich
Copy link
Contributor

@svenmuennich svenmuennich commented Dec 5, 2019

PR #2721 introduced support for trailing comments in swiftlint:disable commands. However, if such a trailing comment contains a URL, the command is currently not parsed correctly and as a result is not recognized at all.

This PR fixes parsing swiftlint:disable commands having a URL in their trailing comment, e.g.:

// swiftlint:disable rule_id - See https://github.com/realm/SwiftLint/pull/2985

@svenmuennich svenmuennich force-pushed the svenmuennich/fix-rule-disable-comment-with-url branch from 9ccb9de to 0e600df Compare December 5, 2019 09:10
@SwiftLintBot
Copy link

SwiftLintBot commented Dec 5, 2019

12 Messages
📖 Linting Aerial with this PR took 1.2s vs 1.18s on master (1% slower)
📖 Linting Alamofire with this PR took 2.19s vs 2.19s on master (0% slower)
📖 Linting Firefox with this PR took 8.86s vs 8.9s on master (0% faster)
📖 Linting Kickstarter with this PR took 14.31s vs 14.37s on master (0% faster)
📖 Linting Moya with this PR took 1.16s vs 1.16s on master (0% slower)
📖 Linting Nimble with this PR took 1.36s vs 1.39s on master (2% faster)
📖 Linting Quick with this PR took 0.53s vs 0.53s on master (0% slower)
📖 Linting Realm with this PR took 2.39s vs 2.35s on master (1% slower)
📖 Linting SourceKitten with this PR took 0.98s vs 0.98s on master (0% slower)
📖 Linting Sourcery with this PR took 6.65s vs 6.72s on master (1% faster)
📖 Linting Swift with this PR took 12.22s vs 12.23s on master (0% faster)
📖 Linting WordPress with this PR took 14.57s vs 14.64s on master (0% faster)

Generated by 🚫 Danger

return match(pattern: pattern, with: [.comment], range: range).compactMap { range in
return Command(string: contents, range: range)
return match(pattern: pattern, range: range).filter { match in
return Set(match.1).isSubset(of: [.comment, .commentURL])
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should use SyntaxKind.commentKinds just to be safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SyntaxKind.commentKinds contains

  • .comment,
  • .commentUrl,
  • .docComment,
  • .docCommentField, and
  • .commentMark.

Changing this as proposed would allow adding SwiftLint commands to doc comments as well as MARK: comments, which is currently not supported. I'm not sure whether such a change should be part of this PR.

@@ -14,7 +14,8 @@

#### Bug Fixes

* None.
* Fix parsing of SwiftLint commands containing a URL in their trailing comment.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please rebase and fix this conflict? Thanks!

Copy link
Contributor Author

@svenmuennich svenmuennich Jan 3, 2020

Choose a reason for hiding this comment

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

I merged rebased on master and resolved this conflict.

@svenmuennich svenmuennich force-pushed the svenmuennich/fix-rule-disable-comment-with-url branch from 7069f8a to e0205d8 Compare January 3, 2020 12:31
@jpsim
Copy link
Collaborator

jpsim commented Jan 3, 2020

Lovely, thank you for the fix!

@jpsim jpsim merged commit fb5361e into realm:master Jan 3, 2020
@svenmuennich svenmuennich deleted the svenmuennich/fix-rule-disable-comment-with-url branch January 6, 2020 09:36
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.

4 participants