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

Introduce " - " delimiter to allow rules to be commented #2721

Merged
merged 2 commits into from
May 13, 2019
Merged

Introduce " - " delimiter to allow rules to be commented #2721

merged 2 commits into from
May 13, 2019

Conversation

kevinrandrup
Copy link
Contributor

Introduce "-" delimiter to allow rules to be commented

Ex. swiftlint:disable:next force_try - Explanation here

@kevinrandrup
Copy link
Contributor Author

kevinrandrup commented Apr 15, 2019

I opened this PR as a result of #2623. Currently figuring out how Github forks work with rebasing. Rebased

@SwiftLintBot
Copy link

SwiftLintBot commented Apr 15, 2019

12 Messages
📖 Linting Aerial with this PR took 3.16s vs 3.16s on master (0% slower)
📖 Linting Alamofire with this PR took 5.05s vs 4.99s on master (1% slower)
📖 Linting Firefox with this PR took 14.95s vs 14.9s on master (0% slower)
📖 Linting Kickstarter with this PR took 26.83s vs 26.81s on master (0% slower)
📖 Linting Moya with this PR took 2.45s vs 2.26s on master (8% slower)
📖 Linting Nimble with this PR took 2.21s vs 2.23s on master (0% faster)
📖 Linting Quick with this PR took 0.79s vs 0.81s on master (2% faster)
📖 Linting Realm with this PR took 3.72s vs 3.7s on master (0% slower)
📖 Linting SourceKitten with this PR took 1.48s vs 1.49s on master (0% faster)
📖 Linting Sourcery with this PR took 4.9s vs 4.91s on master (0% faster)
📖 Linting Swift with this PR took 38.24s vs 37.88s on master (0% slower)
📖 Linting WordPress with this PR took 29.18s vs 29.25s on master (0% faster)

Generated by 🚫 Danger

@kevinrandrup
Copy link
Contributor Author

Question for whoever will review this PR: where do documentation changes need to be made?

The only place I've potentially found is SuperfluousDisableCommandRule.description

@kevinrandrup
Copy link
Contributor Author

Is there anything I can do to move this PR forward?

@jpsim
Copy link
Collaborator

jpsim commented Apr 29, 2019

Thanks for the PR, @kevinrandrup, this is a pretty useful feature!

I know the suggested syntax is currently triggering a superfluous disable command warning, but that seems accidental more than related to the feature at all.

So I wouldn't do anything with SuperfluousDisableCommandRule at all, like adding the delimiter.

I'd rather see that added to the Command struct, ideally even parsing the text after the delimiter, perhaps as a trailingComment property of the command.

What do you think?

@kevinrandrup
Copy link
Contributor Author

kevinrandrup commented Apr 30, 2019

@jpsim Thanks for the feedback. I've updated the PR to parse extra text as Command.trailingComment. I still need to update a unit tests to verify that property but the parsing works. Updated.

Ex. swiftlint:disable:next force_try - Explanation here
@kevinrandrup kevinrandrup changed the title Introduce "-" delimiter to allow rules to be commented Introduce " - " delimiter to allow rules to be commented Apr 30, 2019
@kevinrandrup
Copy link
Contributor Author

@jpsim Anything else you need me to do?

@jpsim
Copy link
Collaborator

jpsim commented May 13, 2019

Nothing further to do, changes look good. Thanks @kevinrandrup!

@jpsim jpsim merged commit d743941 into realm:master May 13, 2019
@kevinrandrup kevinrandrup deleted the kevinrandrup/add-swiftlint-rule-delimiter branch May 13, 2019 19:25
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.

3 participants