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

Rule Request: Warning for extensions used for grouping code #1767

Closed
kurabi opened this Issue Aug 10, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@kurabi

kurabi commented Aug 10, 2017

Swift Extensions are often times used in 3 ways (there are more):

  1. Retroactive Modeling
  2. Protocol Extensions
  3. For organization purposes, where grouping a set of methods together within the same file that declared that class. For example, grouping private methods in an extension, or an extension to implement each protocol that the class implements.

The use of swift extensions for with regards to point 3 above is very trendy right now and I feel strongly that it is incorrect and has drawbacks. A simple // MARK: achieves the “code organization” intention of using extensions.

Reasons to not use extensions for organizing code within the same file:

  • Unlike MARKs, they do not show up in the source editor summary dropdown in Xcode. (Screenshot: https://goo.gl/T5vkr7)
  • Must scroll through the file to find all protocol conformances
  • Extensions obviously can’t override methods, hence this limits how these groups are composed
  • Can’t add properties to extensions, so grouping is somewhat false and not always fully contained as they may suggest
  • Take more space than MARKs and not as concise
  • Lead to longer compile times (I am finding a source for this)
  • Myth: Extensions don’t fully force protocol conformance to be in the scope of that extension
  • Myth: They do not give ability to implement protocol privately

Objective-c has categories offer ability to be named yet this pattern isn’t trendy there, suddenly its trendy in Swift.

Extensions are awesome, so to be clear, this proposed swift lint rule would only apply to this trendy usage of extensions for the above drawbacks mentioned above.

@kurabi kurabi changed the title from Rule Request: Extensions that are used for Grouping Code to Rule Request: Warning for extensions used for grouping code Aug 10, 2017

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Aug 10, 2017

Collaborator

This should be a good starter rule if anyone wants to grab it (as long as it catches only extensions/classes in a single file). It definitely should be opt-in though.

Collaborator

marcelofabri commented Aug 10, 2017

This should be a good starter rule if anyone wants to grab it (as long as it catches only extensions/classes in a single file). It definitely should be opt-in though.

@Uncommon

This comment has been minimized.

Show comment
Hide comment
@Uncommon

Uncommon Aug 10, 2017

Contributor

So what would you want the rule to catch? Extensions on a class in the same file where the class is declared?

Contributor

Uncommon commented Aug 10, 2017

So what would you want the rule to catch? Extensions on a class in the same file where the class is declared?

@kurabi

This comment has been minimized.

Show comment
Hide comment
@kurabi

kurabi commented Aug 10, 2017

@Uncommon Yes.

@Mazyod

This comment has been minimized.

Show comment
Hide comment
@Mazyod

Mazyod Aug 20, 2017

Contributor

I thought about triggering for multiple protocol extensions, but then realized it would require checking for any possible specialization that would require extensions to be split up... Seemed not worth investing in right now.

Contributor

Mazyod commented Aug 20, 2017

I thought about triggering for multiple protocol extensions, but then realized it would require checking for any possible specialization that would require extensions to be split up... Seemed not worth investing in right now.

@ileitch

This comment has been minimized.

Show comment
Hide comment
@ileitch

ileitch Sep 4, 2017

@marcelofabri I don't believe this was implemented as @kurabi requested. The request seems to be to only trigger on groupings that are purely organizatonal extensions, without any protocol conformance, e.g:

class Foo {
}

// MARK: - Private 
extension Foo {
}

The rule shipped in 0.22 matches any extension.

ileitch commented Sep 4, 2017

@marcelofabri I don't believe this was implemented as @kurabi requested. The request seems to be to only trigger on groupings that are purely organizatonal extensions, without any protocol conformance, e.g:

class Foo {
}

// MARK: - Private 
extension Foo {
}

The rule shipped in 0.22 matches any extension.

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Sep 4, 2017

Collaborator

He wrote:

For example, grouping private methods in an extension, or an extension to implement each protocol that the class implements.

Anyway, feel free to submit a PR with this as a configuration if you'd like 👍

Collaborator

marcelofabri commented Sep 4, 2017

He wrote:

For example, grouping private methods in an extension, or an extension to implement each protocol that the class implements.

Anyway, feel free to submit a PR with this as a configuration if you'd like 👍

@ileitch

This comment has been minimized.

Show comment
Hide comment
@ileitch

ileitch Sep 4, 2017

ah yes, I must have glazed over that part 😟

ileitch commented Sep 4, 2017

ah yes, I must have glazed over that part 😟

@kurabi

This comment has been minimized.

Show comment
Hide comment
@kurabi

kurabi Sep 5, 2017

@ileitch The intention was to also flag extensions that implement a protocol too because of the same drawbacks (if not more) as mentioned above.

kurabi commented Sep 5, 2017

@ileitch The intention was to also flag extensions that implement a protocol too because of the same drawbacks (if not more) as mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment