Skip to content

[ObjC] generated method name's WithCompletionBlock postfix doesn't follow convention? #2121

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

Closed
hideya opened this issue Feb 12, 2016 · 8 comments

Comments

@hideya
Copy link
Contributor

hideya commented Feb 12, 2016

API methods for Obj-C always seems to have WithCompletionBlock postfix, which doesn't follow the coding convention. For example, I think the following generated method:

-(NSNumber*) userConfirmWithCompletionBlock :(NSString*) uid 
     token:(NSString*) token 
     redirect:(NSString*) redirect 
    completionHandler: (void (^)(NSError* error))completionBlock;

should have been like the following:

-(NSNumber*) userConfirmWithUid :(NSString*) uid 
     token:(NSString*) token 
     redirect:(NSString*) redirect 
    completionHandler: (void (^)(NSError* error))completionBlock;

FYI, the yaml for this method is:

  /Users/confirm:
    get:
      tags:
        - User
      summary: Confirm a user registration with email verification token.
      description: !<tag:yaml.org,2002:js/undefined> ''
      operationId: User.confirm
      parameters:
        - name: uid
          in: query
          description: !<tag:yaml.org,2002:js/undefined> ''
          required: true
          type: string
        - name: token
          in: query
          description: !<tag:yaml.org,2002:js/undefined> ''
          required: true
          type: string
        - name: redirect
          in: query
          description: !<tag:yaml.org,2002:js/undefined> ''
          required: false
          type: string
      responses:
        '204':
          description: Request was successful
          schema: !<tag:yaml.org,2002:js/undefined> ''
      deprecated: false

This is as of b54947d (the current latest) (@bajtos wanted to keep informed about this too)

@wing328
Copy link
Contributor

wing328 commented Feb 12, 2016

@hideya do you mean the postfix (WithCompletionBlock) is causing compiling error?

@wing328 wing328 added this to the v2.1.6 milestone Feb 12, 2016
@hideya
Copy link
Contributor Author

hideya commented Feb 12, 2016

@wing328 no, this is an improvement request, as e.g. userConfirmWithCompletionBlock doesn't follow Obj-C method naming convention. Sorry for being unclear.

@hideya hideya changed the title [ObjC] generated method name's WithCompletionBlock postfix doesn't look correct? [ObjC] generated method name's WithCompletionBlock postfix doesn't follow convention? Feb 12, 2016
@wing328
Copy link
Contributor

wing328 commented Feb 12, 2016

I see. For the naming convention, do you have the URL to the style guide for our reference?

@hideya
Copy link
Contributor Author

hideya commented Feb 12, 2016

Sure. Apple's Guide to Naming Methods says that:

Make the word before the argument describe the argument.

  • (id)viewWithTag:(NSInteger)aTag; -- Right.
  • (id)taggedView:(int)aTag; -- Wrong.

@wing328
Copy link
Contributor

wing328 commented Feb 12, 2016

Thanks for the URL. Using the example User.confirm you provide, you suggest naming it as userConfirmWithUid where uid is the first argument (in Pascal case).

The easiest fix I can think of is to rename operationId to userConfirmWithUid but of course the side effect is that other API clients would use the same method name (assuming API client in other langauges is used)

To fix it just for ObjC, one needs to customize the ObjC generator to provide another mustache tag (e.g. firstParameter), which stores the name of the 1st method argument in Pascal case. This can be done in Map<String, Object> postProcessOperations(Map<String, Object> objs)

@hideya
Copy link
Contributor Author

hideya commented Feb 12, 2016

Thanks for the explanation. Yeah, I'd like to have it fixed just for ObjC.
Would you willing to take the change if I worked on a PR for it? (although it is a compatibility breaking change in terms of ObjC client SDKs people already have generated...)

@wing328
Copy link
Contributor

wing328 commented Feb 12, 2016

I don't mind. To me it's ok to make this breaking change (non-backward compatible) as your suggestion is better in terms of conforming to Apple's guidelines in naming method.

Later we can include this in the release note or migration guide to make sure developers are informed of this breaking change when they upgrade to the next stable release.

@hideya
Copy link
Contributor Author

hideya commented Feb 12, 2016

Thanks! I'll work on it then!

wing328 added a commit that referenced this issue Feb 16, 2016
[ObjC] Fix #2121, generated method names don't follow coding convention
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants