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

Addded TrailingCommaRule #886

Merged
merged 5 commits into from Nov 26, 2016
Merged

Addded TrailingCommaRule #886

merged 5 commits into from Nov 26, 2016

Conversation

marcelofabri
Copy link
Collaborator

Fixes #883.

How would be the best way to handle both styles (mandatory comma and forbidden comma)? A configuration (if so, how should the rule description be)? Another rule?

I made this rule opt-in as even SwiftLint codebase isn't consistent. Thoughts?

PS: The changelog entry is missing on purpose as I thought it'd be better to think on how to handle both styles first.

@codecov-io
Copy link

codecov-io commented Nov 21, 2016

Current coverage is 86.11% (diff: 89.79%)

Merging #886 into master will increase coverage by 0.08%

@@             master       #886   diff @@
==========================================
  Files           112        114     +2   
  Lines          4959       5055    +96   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4266       4353    +87   
- Misses          693        702     +9   
  Partials          0          0          

Powered by Codecov. Last update ad019af...50ec4e3

@jpsim
Copy link
Collaborator

jpsim commented Nov 25, 2016

I think this rule can be on-by-default as long as it's also configurable.

@marcelofabri
Copy link
Collaborator Author

But how would I handle the configurations in the examples? They're very different according to the specified configuration.

@jpsim
Copy link
Collaborator

jpsim commented Nov 25, 2016

But how would I handle the configurations in the examples? They're very different according to the specified configuration.

True, but that wouldn't be unprecedented. We have other rules where we reuse the convenient RuleDescription examples for tests with the default configuration, but have more explicit tests for custom configurations. For example, see RulesTests.testColon() where we first run the standard verifyRule(ColonRule.description) test, followed by passing in a custom RuleDescription and configuration: verifyRule(description, ruleConfiguration: ["flexible_right_spacing": true]).

Could do the same here, although there's an argument to be made that we should have a nicer way to specify different test examples for each configuration to test.

@marcelofabri
Copy link
Collaborator Author

@jpsim Another (small) problem with the current description behavior is that swiftlint rules [rule] would always show the examples for the default configuration, but it could be interesting to show for the provided one instead. However I do feel that this feature is mostly unused and few people are aware of it.

Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

Looks great! I've given you write access, since you've done nothing but great contributions! So merge this whenever you feel it's ready.


* Add `TrailingCommaRule` to enforce/forbid trailing commas in arrays and
dictionaries. The default is to forbid them, but this can be changed with
the `mandatory_comma` configuration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

dictionaries. The default is to forbid them, but this can be changed with
the `mandatory_comma` configuration.
[Marcelo Fabri](https://github.com/marcelofabri)
[#886](https://github.com/realm/SwiftLint/issues/886)
Copy link
Collaborator

Choose a reason for hiding this comment

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

link to the issue this resolves, not the PR adding it: https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

@marcelofabri
Copy link
Collaborator Author

73455700

@marcelofabri marcelofabri merged commit b22d1a9 into realm:master Nov 26, 2016
@marcelofabri marcelofabri deleted the trailing-comma branch November 26, 2016 12: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.

None yet

3 participants