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

Use Base Class Rule #84

Merged
merged 11 commits into from Mar 12, 2019

Conversation

Projects
None yet
2 participants
@masamichiueta
Copy link
Contributor

commented Mar 11, 2019

This PR is a proposal of a new rule, UseBaseClass.

What is UseBaseClass rule?

UseBaseClass rule is a rule to check whether an UI element uses custom class which is set as base class in config.

For example, if we have some base classes of UILabel like PrimaryLabel and SecondaryLabel that should be applied to all labels through the project, this rule checks whether labels in storyboards and xibs are set to base classes.

Why is this rule required?

As I describe above, many apps commonly have their own base UI components. But we often forget to set the custom class to the base classes in storyboard or xib. And new members often overlook the base components.

This rule can prevent the problem and this is the reason why I developed this rule.

How does it work?

Set element_class and base_classes in config like

use_base_class_rule:
  - element_class: UILabel
    base_classes:
      - PrimaryLabel
      - SecondaryLabel 

This enables to check UILabel uses PrimaryLabel or SecondaryLabel. If UILabel does not use the base classes, Xcode shows warnings like below.

Screen Shot 2019-03-11 at 18 53 35

@kateinoigakukun kateinoigakukun self-requested a review Mar 11, 2019

@kateinoigakukun
Copy link
Member

left a comment

Thank you for your contribution 🎉
Looks almost OK to me. But this rule can't work for inherited class because IB doesn't know about Swift type information.
So please add that caution on rule description of README.

return views.flatMap { validate(for: $0.view, file: xib, fileNameWithoutExtension: xib.fileNameWithoutExtension) }
}

private func validate<T: InterfaceBuilderFile>(for view: ViewProtocol, file: T, fileNameWithoutExtension: String) -> [Violation] {

This comment has been minimized.

Copy link
@kateinoigakukun

kateinoigakukun Mar 11, 2019

Member

Is fileNameWithoutExtension needed?

This comment has been minimized.

Copy link
@masamichiueta

masamichiueta Mar 11, 2019

Author Contributor

Oh, it's not used anywhere😅 I can remove it!

case baseClasses = "base_classes"
}

public static let `default` = UseBaseClassConfig.init()

This comment has been minimized.

Copy link
@kateinoigakukun

kateinoigakukun Mar 11, 2019

Member

I think this default and private init is not used anywhere. Can you remove them?

This comment has been minimized.

Copy link
@masamichiueta

masamichiueta Mar 11, 2019

Author Contributor

I see👍 Thanks!


public init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
elementClass = try container.decodeIfPresent(Optional<String>.self, forKey: .elementClass).flatMap { $0 } ?? ""

This comment has been minimized.

Copy link
@kateinoigakukun

kateinoigakukun Mar 11, 2019

Member

If elementClass is nil or not present, this should throw a decoding error.
But in the case of baseClasses, it's OK because empty array in YAML is expressed in the same way of nil

elementClass: # This is invalid. Shouldn't be nil
baseClasses: # It's OK. This means empty array

This comment has been minimized.

Copy link
@masamichiueta

masamichiueta Mar 11, 2019

Author Contributor

I'll fix to throw an error!

@masamichiueta

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@kateinoigakukun Thank you for your quick review!

this rule can't work for inherited class because IB doesn't know about Swift type information

[Q] What do you mean about inherited class?

@kateinoigakukun

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@masamichiueta Oh, I misunderstood 😅. I mean PrimaryLabel can be inherited.
Do you think that CustomizedPrimaryLabel should be invalid? If so, it's my misunderstanding and you don't need to change README

class PrimaryLabel: UILabel {
    ...
}

class CustomizedPrimaryLabel: PrimaryLabel {
    ... 
}
@masamichiueta

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I understand! As you said, this rule does not work for inherited classes. A developer needs to add the inherited classes to base_classes in yml.

I add caution on rule description👍

masamichiueta added some commits Mar 12, 2019

@masamichiueta

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Updated. Please review again🙏

@kateinoigakukun
Copy link
Member

left a comment

OK, LGTM!

@kateinoigakukun kateinoigakukun merged commit 316dea3 into IBDecodable:master Mar 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.