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

Custom CheckStyle: Batch 2, Rule #5 and #10; Batch 3, Rule #1 and #6 #4712

Merged
merged 38 commits into from
Aug 28, 2019

Conversation

mssfang
Copy link
Member

@mssfang mssfang commented Jul 30, 2019

Batch 2

  • Rule # 5
    Service client methods: All methods that are in a class annotated with @ServiceClient, where the method has a @ServiceMethod annotation, should follow these rules:

    • Methods names should follow a common vocabulary. Refer to Java spec for this naming pattern https://azure.github.io/azure-sdk/java_design.html.
    • Method names imply certain rules around expected return type - these should all be validated.
    • Methods should not have 'Async' added to their method name.
    • Return types of async and sync clients should be as per guidelines: [this check will be ignored for now since I am still struggling on how to get the Reflection working in the code-quality-check]
      • Return type for async collection should be of type ? extends PagedFlux
      • Return type for async single value should be of type ? extends Mono
      • Return type for sync collection should be of type ? extends PagedIterable
      • Return type for sync single value should be of type ? extends Response
  • Rule # 10
    'withResponse' naming pattern: All methods annotated with @ServiceMethod that return a Response (or Mono) must have their method name end with 'withResponse'. If the service method does not return a Response or Mono, it must not end with 'withResponse'.

Batch 3

  • Rule # 1
    Context in all the right places: Context should be passed in as an argument to all public methods annotated with @ServiceMethod that return Response in sync clients.

    • Only in the sync case: E.g. we want this: Public Response getFooWithResponse(int x, int y, Context c)
    • In the async case, we should check to ensure we have no service methods that take Context!
  • Rule # 6
    Async client should have async = true property set in @ServiceClient annotation

    • To validate this, if the class has @ServiceClient annotation and if the classname AsyncClient, verify that the async property of @ServiceClient annotation is set to true.
    • Similarly, if the class has @ServiceClient annotation and if the classname is Client, verify that the async property of @ServiceClient annotation is set to false
  • Change class Context to be final

@mssfang mssfang self-assigned this Jul 30, 2019
@mssfang mssfang changed the title Custom CheckStyle Rule B #5 and #10 Custom CheckStyle Rule #5 and #10 Jul 30, 2019
@mssfang mssfang changed the title Custom CheckStyle Rule #5 and #10 Custom CheckStyle: Batch 2, Rule #5 and #10; Batch 3, Rule #1 Jul 31, 2019
@mssfang mssfang changed the title Custom CheckStyle: Batch 2, Rule #5 and #10; Batch 3, Rule #1 Custom CheckStyle: Batch 2, Rule #5 and #10; Batch 3, Rule #1 and #6 Jul 31, 2019
@mssfang mssfang marked this pull request as ready for review August 9, 2019 00:53
Copy link
Member

@vhvb1989 vhvb1989 left a comment

Choose a reason for hiding this comment

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

asking just for a few code enhacements

@@ -168,7 +187,8 @@ private void checkClassField(DetailAST objBlockToken) {
// VARIABLE_DEF token will always MODIFIERS token. If there is no modifier at the variable, no child under
// MODIFIERS token. Also the previous sibling of OBJBLOCK will always be class name IDENT node.
if (!modifiersToken.branchContains(TokenTypes.FINAL)) {
log(modifiersToken, String.format("The variable field ''%s'' of class ''%s'' should be final. Classes annotated with @ServiceClient are supposed to be immutable.",
Copy link
Member

Choose a reason for hiding this comment

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

update this for to use TokenUtil.forEachChild(token, condition, predicate -> ….)

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure why to use forEachChild here.

@mssfang mssfang requested a review from vhvb1989 August 20, 2019 23:59
@mssfang mssfang requested a review from conniey August 28, 2019 03:13
Copy link
Member

@conniey conniey left a comment

Choose a reason for hiding this comment

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

Some feedback to address. Should be easy.

@mssfang mssfang merged commit 66fd3cd into Azure:master Aug 28, 2019
@mssfang mssfang deleted the CS-Service-Method branch August 28, 2019 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants