Issues/5530 use consistent naming #6054
Conversation
|
Can one of the admins verify this patch? |
| <!-- 4. longerVariableOrMethodName --> | ||
| <!--module name="com.azure.tools.checkstyle.checks.BlacklistedWordsCheck"> | ||
| <property name="blacklistedWords" value="URL, HTTP, XML, JSON, SAS, CPK, API" /> | ||
| </module--> |
There was a problem hiding this comment.
Will uncomment this once the naming issues have been fixed. Otherwise it'll break the build.
| /** | ||
| * Should we check member with given modifiers. | ||
| * | ||
| * @param token |
There was a problem hiding this comment.
nit: is it format better if keeping
@param token modifiers of the member to check.There was a problem hiding this comment.
Sounds good. I also saw the other note about the same issue on another method.
mssfang
left a comment
mssfang
left a comment
There was a problem hiding this comment.
Just a minor nit on formatting. LGTM as well!.
|
@JonathanGiles said only Public API's should be checked. What is considered public? Anything under sdk? Should the sample code also be checked? |
Anything consumed by .\pom.client.xml should be fixed. We're more lax about test code with bad naming, but I dont' think it's a dev burden to ensure our samples look right. . . And if we tap into the idealist in me, I'd hope that the test code also was fixed. |
|
I can fix the tests as well but someone already asked me to suppress the check for them. Should I check them as well? If you're unsure, worst case scenario someone just removes the suppression and fixes them later. |
|
@chrischild |
|
47 files without any suppressions. Suppressing src\main\java\com\azure\core\test\implementation\entities\HttpBinFormDataJSON.java |
...e-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/BlacklistedWordsCheck.java
Outdated
Show resolved
Hide resolved
...e-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/BlacklistedWordsCheck.java
Outdated
Show resolved
Hide resolved
|
Just popping in to see what's happening. I was looking over the report above and noticed that the |
Yes, suppress anything under implementation. Thanks. |
|
OK. This means there are no changes that come from running the checkstyle right now using the suppressions |
|
Hello again - I checked in the last changes that were requested and haven't heard anything about the previous post I left regarding the packages I was told to suppress. At this point I think this issue is complete. |
|
@chrischild I apologise for the delayed responses, I don't get updates on pushed commits. Thanks so much. :) |
|
@conniey No worries. If you do find that you would like some of those packages removed from the suppressions and have those errors fixed, do not hesitate to mention me in the issue and I'll gladly take a look. |
5 participants
I'm sorry I had to delete the last branch. I found out I had been committing some info I didn't want to be and then when I went to re-open the issue I wasn't allowed to.
Old PR is here
Fixes #5530