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

Remove MARK rules #26

Closed
wants to merge 1 commit into from
Closed

Remove MARK rules #26

wants to merge 1 commit into from

Conversation

fdiaz
Copy link
Collaborator

@fdiaz fdiaz commented Nov 8, 2018

Summary

Removes both rules about using MARK

Reasoning

  1. This rule is not lintable
  2. Seems like there's disagreement in the team whether this rule is useful or not.

Reviewers

cc @airbnb/swift-styleguide-maintainers

Please react with 👍/👎 if you agree or disagree with this proposal.

@bachand
Copy link
Contributor

bachand commented Nov 8, 2018

@fdiaz I disagree that this rule isn’t lintable. We could make a PR to Swiftlint.

I support revisiting these rules but I think deleting them outright without trying to distill some of the thinking into more straightforward rules is a mistake.

For example, I see value in having members of similar access control being co-located, within a type which we get from one of these rules. I’d like to keep that, even if the new rules is “co-locate members of similar access control” with a particular order specified.

I also find the MARKs for types and protocol extensions to be useful. I feel less strongly about MARKs for different levels of access control.

@bachand
Copy link
Contributor

bachand commented Nov 8, 2018

I also like that we’ve recommended not having free-from MARKs:

Do not subdivide each of these sections into subsections, as it makes the method dropdown more cluttered and therefore less useful. Instead, group methods by functionality and use smart naming to make clear which methods are related. If there are enough methods that sub-sections seem necessary, consider refactoring your code into multiple types.

@dfed
Copy link
Contributor

dfed commented Nov 9, 2018

Given that SwiftLint allows for regular expressions, it should absolutely be possible to lint for

Each type in a file should be preceded by `// MARK: - TypeName`

I'd bet the following is also lint-able with a clever regular expression (which would of course have to get more clever if combined with the above and below).

Use `// MARK:` to separate the contents of a type definition into the sections listed below, in order.

And while I think the following is also doable with a regular expression (with some caveats), I think it might be good to move the removal of it into a separate PR. While it touches MARKs, it primarily touches protocol conformance

Each protocol conformance implementation should occur in dedicated type extension within the same file as the type

@falkovazeem
Copy link

Regardless of lintability, I feel that this rule is not useful and should be removed. It clutters our code needlessly for readable sections, and the forced grouping by access control is not readable or maintainable.

@dfed
Copy link
Contributor

dfed commented Nov 9, 2018

@P8SrjpWtW8fX re whether we should remove this rule on the merits, PTAL at my comment here:
#28 (comment)

I think we should keep merit-based removal arguments out of these PRs, since the goal of these PRs (I believe) is to make the rules of our style guide more capable of being linted

@fdiaz
Copy link
Collaborator Author

fdiaz commented Nov 14, 2018

Closing for now until we agree on the tenets we want re: linting. I'll add a PR with a proposal soon.

@fdiaz fdiaz closed this Nov 14, 2018
@fdiaz fdiaz deleted the fd/remove-mark-rule branch November 14, 2018 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants