Allow category method name prefix to be specified. #32

ebaker355 · 2016-01-30T20:01:41Z

This PR allows the category method name prefix to be set to something other than ac. When the prefix matches what other categories are using in a project, it makes it clear that the category belongs to that project, and was not imported via an external framework. Also, in the case where several asset catalogs exist in a project, it can be useful to use a different prefix for each one. If no prefix is specified, the default ac is used.

commit 8f6d5c8 Merge: 360bc82 6b378e8 Author: Baker, Eric (MONHC-C) <eric.baker@hyatt.com> Date: Fri Jan 29 15:27:54 2016 -0600 Merge branch 'feature/specify-prefix' into develop commit 6b378e8 Author: Baker, Eric (MONHC-C) <eric.baker@hyatt.com> Date: Fri Jan 29 15:26:26 2016 -0600 Add method name prefix option. commit 0ed5396 Author: Baker, Eric (MONHC-C) <eric.baker@hyatt.com> Date: Fri Jan 29 11:51:20 2016 -0600 Rename FrameworkPrefix to MethodNamePrefix

# Conflicts: # Cat2Cat/main.m

designatednerd · 2016-01-31T17:16:12Z

Hey @ebaker355: I like this idea! Do you mind updating the sample projects so we can verify the tests on those pass with this new change? Got a couple other questions I'll ask inline.

designatednerd · 2016-01-31T17:19:39Z

README.md

  ```
  --asset-catalog="Resources/Images.xcassets"
  ```  
  ```
  --asset-catalog="Resources/Media.xcassets"
  ```
+- The prefix for the category method names. If not specified, the default is `ac`. Must be at least 2 characters, and conform to valid method naming rules:


One thing you mention is possible in your PR comment but isn't quite clear in this documentation: What happens if multiple asset catalogs are specified?

Do you need to add multiple prefixes for the method name? Is the first method name prefix passed in used for every catalog? How do you specify which prefix is for which catalog?

These are good questions that I hadn't thought of.

I suppose the prefixes should be added to an array, just like the asset names are, and applied in the same order as on the command line. I'm thinking that in the case where more catalogs are specified than prefixes, the last prefix specified will be used for the remaining catalogs, for the case where all catalogs should share the same prefix; on the other hand, more prefixes than catalogs will result in the leftover prefixes being ignored. Do you agree? Or have another ideas?

To complicate things a bit, glob patterns (i.e., shell wildcard type patterns) can be passed as asset catalog names, so a single asset catalog parameter can turn into multiple asset catalogs.

Hmm, that's a tricky one. However, if someone is using glob patterns in a run script build phase, I'd think they're not overly worried about assigning specific prefixes, since the processing order is essentially undefined. If they wanted to ensure a specific prefix matches a certain asset catalog, they should probably be specific about both in the command line, imho.

I'm wondering if maybe we're making this too complicated—maybe Cat2Cat should get invoked separately for each prefix? That is, if you've got some stuff with prefix abc and some with prefix xyz, you should run Cat2Cat 2 separate times? That'd avoid all the issues about aligning different asset catalogs with different prefixes.

Yeah... in fact, when I opened the sample projects to start writing tests for this PR, I noticed that the categories files would simply be overwritten, unless there was a way to specify a different name for the category as well.

Maybe I've opened a can of worms here... 😿

I think having the category names be prefixed isn't really a bad idea and would fix that.

Ok, but... how do we apply the prefixes for each usage? For example, in our project (which I think is following pretty standard convention), we prefix our category methods with xyz_, but our category names are prefixed with XYZ. So, our categories would look like:

UIImage+XYZAssetCatalog.h/m and
+ (UIImage *)xyz_SomeImage;

Would we accept only one prefix and mutate it to match these usages, forcing this convention? Or do we allow separate prefixes for methods and categories names to be specified?

If I were using it, I'd want one prefix, forced lowercase and separated with _ for methods and forced uppercase for the category... so I'm thinking either:

just the one parameter and enforce that usage; or

have one parameter that'd enforce that usage and two optional parameters to override just the method prefix and just the other prefix.

Ok, I will update this PR with what we've discussed so far, and we can go from there. Thanks again you guys for all the time you've spent thinking this thru! :)

ebaker355 · 2016-02-01T00:57:02Z

@designatednerd My iOS team lead is very interested in using Cat2Cat for our project. However, there are a few updates he would like us to make. One was in this PR, the ability to specify the category prefix.

Unrelated to this PR, another change we'd like to explore is the ability to generate factory classes instead of categories on UIImage. However, I'm not sure that's a great fit for Cat2Cat. It seems to me that perhaps that should be a different project. Any thoughts? Thanks!

vokal-isaac · 2016-02-01T02:02:02Z

@designatednerd Given what @ebaker355 is looking to do, I'm thinking maybe we'd want to work this like that other tool we're using internally, where the template applied is arbitrary, the platform/language flags select the template(s) to use, and there's a --template parameter that'll take an arbitrary template? If we did that, generating factory classes or pretty much anything else would be a matter of creating the templates and passing them in to Cat2Cat.

designatednerd · 2016-02-01T05:33:34Z

@vokal-isaac Ooh, yah, I like that idea too. Let's discuss tomorrow.

@ebaker355: Give us a day to mull it over and we'll give you a heads up.

ebaker355 · 2016-02-01T12:13:05Z

@designatednerd & @vokal-isaac No problem guys! Thanks for taking the time to consider these ideas. We're not in a huge rush, and we want to ensure any changes are beneficial for everyone.

vokal-isaac · 2016-02-01T16:13:23Z

Cat2Cat/main.m

+           description:@"Prefix for method names (defaults to ac)"
+     blockWithArgument:^(NSString *value) {
+        methodNamePrefix = [value copy];
+    }];


Is there a reason this needs to be done with a block?

I wanted to be able to perform a check for nil on the variable. BRLOptionParser didn't seem to be happy if I passed in a pointer to nil. A nil check was needed to bypass the validation check, if the parameter wasn't specified.

Is that to avoid specifying the default value on the variable declaration?

Correct, since its an optional arg.

I suppose the default "ac" could just be used as the default value, but that constant was defined lower in the code base, and I didn't know if I should move it.

I think, given that you're stating the default value in the description, it might as well move up here. I'm inclined to think the simplification of not having to use the block is also worth the move.

Sounds good. I'll make the update.

ebaker355 · 2016-02-11T19:56:03Z

As of now, I am swamped with work. I will reopen this PR when I have time to complete the changes. Hopefully soon!

ebaker355 added 9 commits January 29, 2016 07:54

Ignore AppCode's .idea directory.

360bc82

Rename FrameworkPrefix to MethodNamePrefix

0ed5396

Add method name prefix option.

6b378e8

Merge branch 'feature/specify-prefix' into develop

8f6d5c8

Update Readme. Indentation fix for main.m.

056588d

Merge branch 'develop'

80bb8d1

# Conflicts: # Cat2Cat/main.m

Mark functions static.

e3c2a57

Indentation in Readme

bf64a97

designatednerd reviewed Jan 31, 2016
View reviewed changes

Remove whitespace-only changes.

b5023dd

vokal-isaac reviewed Feb 1, 2016
View reviewed changes

Rename function to exitWithCommandNameAndError

af9e8a7

ebaker355 closed this Feb 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow category method name prefix to be specified. #32

Allow category method name prefix to be specified. #32

ebaker355 commented Jan 30, 2016

designatednerd commented Jan 31, 2016

designatednerd Jan 31, 2016

ebaker355 Feb 1, 2016

vokal-isaac Feb 1, 2016

ebaker355 Feb 1, 2016

vokal-isaac Feb 1, 2016

ebaker355 Feb 1, 2016

vokal-isaac Feb 1, 2016

ebaker355 Feb 1, 2016

vokal-isaac Feb 1, 2016

ebaker355 Feb 1, 2016

ebaker355 commented Feb 1, 2016

vokal-isaac commented Feb 1, 2016

designatednerd commented Feb 1, 2016

ebaker355 commented Feb 1, 2016

vokal-isaac Feb 1, 2016

ebaker355 Feb 1, 2016

vokal-isaac Feb 1, 2016

ebaker355 Feb 1, 2016

vokal-isaac Feb 1, 2016

ebaker355 Feb 1, 2016

ebaker355 commented Feb 11, 2016

Allow category method name prefix to be specified. #32

Allow category method name prefix to be specified. #32

Conversation

ebaker355 commented Jan 30, 2016

designatednerd commented Jan 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebaker355 commented Feb 1, 2016

vokal-isaac commented Feb 1, 2016

designatednerd commented Feb 1, 2016

ebaker355 commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebaker355 commented Feb 11, 2016