Skip to content

Conversation

@mssfang
Copy link
Contributor

@mssfang mssfang commented Jul 26, 2019

A fix to No External Dependency check style rule. Previously, it fails only on the method's return type but not parameter types, and not checking on a protected class.

Also, remove unused import

@alzimmermsft
Copy link
Member

Does this check handle a scenario such as public void someMethod(Function<ByeBuf, OutputStream> aFunctionINeed)?

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.

minor things , open to talk about it 👍

@mssfang mssfang requested a review from conniey July 26, 2019 23:43
@JonathanGiles
Copy link
Member

Does this check handle a scenario such as public void someMethod(Function<ByeBuf, OutputStream> aFunctionINeed)?

Alan's point here is a good one. For example, ConfigurationClientCredentials has methods that take a Flux<ByteBuf> that should not be allowed. Please ensure that this is tested in this check too.

Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

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

Looks good to me, after taking into account the feedback from the other reviewers.

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.

Looking great!

@mssfang
Copy link
Contributor Author

mssfang commented Jul 29, 2019

Refactored the code and created a recursion method that takes care of (1) TYPE as a root node, it can have IDENT and/or TYPE_AURGUMENTS as children nodes; (2) TYPE_AURGUMENT as a root node, it can have IDENT and/or TYPE_AURGUMENTS as children nodes; (3) TYPE_AURGUMENTS as a root node, it can have TYPE_AURGUMENT as child node.

@mssfang mssfang merged commit 2db7c54 into Azure:master Jul 30, 2019
@mssfang mssfang deleted the CS-Rule-ExternalDependency branch July 30, 2019 00:51
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.

5 participants