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

feat: Add color_resources rule #141

Merged
merged 3 commits into from
Jan 31, 2020

Conversation

r-plus
Copy link
Contributor

@r-plus r-plus commented Jan 27, 2020

This rule is for detect missing named colors similar to image_resources rule.

スクリーンショット 2020-01-27 14 12 17

Comment on lines 53 to 71
private extension AssetsCatalog {
// namespaced names of the
var values: [String] {
return entries.flatMap { $0.values }
}
}

private extension AssetsCatalog.Entry {
var values: [String] {
switch self {
case .group(_, let items):
return items.flatMap { $0.values }
case .color(_, let value):
return [value]
case .image(_, let value):
return [value]
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These private extensions are copied from ImageResourceRule.swift.
What do you think about best position of this extensions?

  1. keep private extensions in each files.
  2. escalate to internal extension and move to inside of AssetsCatalog.swift

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, xcassets can have same name different assets entry (for example, image and color).
We should filter to only color assets for this rule. (and also filter to image for image_resources rule)

okey, keep private extensions in each files is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okey, fixed for that case. (only for color_resources rule in this PR)

Copy link
Member

Choose a reason for hiding this comment

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

so there is a bug in https://github.com/IBDecodable/IBLinter/blob/master/Sources/IBLinterKit/Rules/ImageResourcesRule.swift#L90

maybe if the code is moved to AssetsCatalog, rename it to colorNames and imageNames to be clear about the purpose of the accessors
so no more copy paste, or adding new case (like color) , which could create bugs

Copy link
Contributor Author

@r-plus r-plus Jan 27, 2020

Choose a reason for hiding this comment

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

Should i fix by moving extension methods to AssetsCatalog.swift and rename to colorNames and imageNames in this PR?

Copy link
Contributor Author

@r-plus r-plus Jan 28, 2020

Choose a reason for hiding this comment

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

@phimage I tried to filter by Item enum argument having method instead of xxxNames methods r-plus@df983e6
What do you think of this strategy? (not yet applied for this PR)

Copy link
Member

Choose a reason for hiding this comment

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

it's more "swifty" and maybe more elegant but
this add just a little runtime cost
so as you want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked how many runtime cost by unit test r-plus@e7d939c

Results in my machine

スクリーンショット 2020-01-29 10 42 31

using enum for filter is about 10% slower than type fixed method. But it is single digit ms level cost in usual size of xcassets. so I want to use swifty way.
is it good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okey, picked that commit.

Copy link
Member

Choose a reason for hiding this comment

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

yes, you can use enum

@phimage phimage merged commit f333295 into IBDecodable:master Jan 31, 2020
@r-plus r-plus deleted the feature/rule/color_resources branch February 1, 2020 02:50
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.

2 participants