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

idea: tenth part of idea violations #4725

Closed
Nimfadora opened this Issue Jul 15, 2017 · 13 comments

Comments

Projects
2 participants
@Nimfadora
Contributor

Nimfadora commented Jul 15, 2017

sub-task of #4681

full report is here

violations reported by TC, but not seen on my local:
JavaScript function metrics
DOM issues
CSS
Code style issues
Code quality tools
General -> Duplicated Code

violations remained undocumented:

GjsLint
JSHint
JSLint
TsLint
Eslint
Jscs
InnerHTMLJS
Duplicates
AssertMessageNotString
CheckStyle
CollectionAddAllCanBeReplacedWithConstructor
CssConvertColorToRgbInspection
CssUnusedSymbol
DanglingJavadoc
ExtendsThrowable
FieldAccessedSynchronizedAndUnsynchronized
FieldRepeatedlyAccessed
FunctionWithMultipleLoopsJS
Guava
GuavaFluentIterable
HtmlUnknownTarget
IgnoreDuplicateEntry
IgnoreRelativeEntry
IgnoreUnusedEntry
JSDeclarationsAtScopeStart
MethodNamesDifferOnlyByCase
MethodOnlyUsedFromInnerClass
PackageNamingConvention enabledByDefault:false
PackageVisibleField
RedundantMethodOverride
ResultOfObjectAllocationIgnored
StaticNonFinalField
UnusedParameters scope:Production:false

@romani romani added this to In Progress in Practice What You Preach Jul 15, 2017

@Nimfadora Nimfadora changed the title from idea: last part of idea violations to idea: tenth part of idea violations Jul 16, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 16, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 16, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 16, 2017

Member

JSHint (1)
JSLint (12)

lets fix this

Variable declarations are at the scope start

suppress: "we like the style when declaration comes close to first usage"

#HEX color representation instead of RGB color function

suppress: "we like hex format of color"

Unused CSS selector

suppress: "most found cases are used in xml files that are source for html pages"

Use of 'innerHTML' property

try to fix.

Duplicated Code

exclude Test scope.
All other cases are reasonable to fix by moving functionality to utils.
Suppress by javadoc tag violations at AbstractComplexityCheck, CyclomaticComplexityCheck (as they caused by deprecated code, one day we will remove them)

Function with multiple loops

suppress: "we do not see a harm from such style"

Call to simple getter from within class

need to be fixed

Collection without initial capacity

suppress: "we do not know initial size in most cases, it could make uncovered mutation problems for pitest that value more"

Method may be 'static'

please activate option to check only private and final methods.
Please do fix but in separate commit , as we will need to open issue on sevntu Check for this as it did not caught such cases, we need to review them later.

Object allocation in loop

suppress: "no options in inspection, there are number of wontfix cases: creation of wrappers, .... "

Single character string concatenation

suppress: "it is not critical parts of our application and benefit is minimal and only for special applications that use such concatenations alot - https://stackoverflow.com/questions/24859500/concatenate-char-literal-x-vs-single-char-string-literal-x"

StringBuffer or StringBuilder without initial capacity

please recheck pitest reaction on this, it is better to have initial values, but if pitest makes mutations that we can not resolve, we need to suppress this inspection.

Hardcoded file separator
Hardcoded line separator

suppress: "there are too much false positives in RegExps and javadoc start/end symbols in paths from classpath, ...."

@NotNull/@Nullable problems

Ok to add annotation.

Constant conditions & exceptions

please exclude Test scope, it should skip all violations.

Magic Constant

suppress: "false positive is reported, we have enough of MagicNumber validations"

Mismatched query and update of collection

try to resolve, reported code is strange.

Member

romani commented Jul 16, 2017

JSHint (1)
JSLint (12)

lets fix this

Variable declarations are at the scope start

suppress: "we like the style when declaration comes close to first usage"

#HEX color representation instead of RGB color function

suppress: "we like hex format of color"

Unused CSS selector

suppress: "most found cases are used in xml files that are source for html pages"

Use of 'innerHTML' property

try to fix.

Duplicated Code

exclude Test scope.
All other cases are reasonable to fix by moving functionality to utils.
Suppress by javadoc tag violations at AbstractComplexityCheck, CyclomaticComplexityCheck (as they caused by deprecated code, one day we will remove them)

Function with multiple loops

suppress: "we do not see a harm from such style"

Call to simple getter from within class

need to be fixed

Collection without initial capacity

suppress: "we do not know initial size in most cases, it could make uncovered mutation problems for pitest that value more"

Method may be 'static'

please activate option to check only private and final methods.
Please do fix but in separate commit , as we will need to open issue on sevntu Check for this as it did not caught such cases, we need to review them later.

Object allocation in loop

suppress: "no options in inspection, there are number of wontfix cases: creation of wrappers, .... "

Single character string concatenation

suppress: "it is not critical parts of our application and benefit is minimal and only for special applications that use such concatenations alot - https://stackoverflow.com/questions/24859500/concatenate-char-literal-x-vs-single-char-string-literal-x"

StringBuffer or StringBuilder without initial capacity

please recheck pitest reaction on this, it is better to have initial values, but if pitest makes mutations that we can not resolve, we need to suppress this inspection.

Hardcoded file separator
Hardcoded line separator

suppress: "there are too much false positives in RegExps and javadoc start/end symbols in paths from classpath, ...."

@NotNull/@Nullable problems

Ok to add annotation.

Constant conditions & exceptions

please exclude Test scope, it should skip all violations.

Magic Constant

suppress: "false positive is reported, we have enough of MagicNumber validations"

Mismatched query and update of collection

try to resolve, reported code is strange.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 16, 2017

Member

Non-final field referenced in 'equals()'
Non-final field referenced in 'hashCode()'

for SuppressElement fix by creation of c-tor that takes all required parameters, all fields should be final.
SuppressionFilter - use javadoc tag suppression, will be investigated at #4734 .

Object comparison using '==', instead of 'equals()'

suppress: "we like comparison of DetailAst by ==, probably one day we will be punished for this"

Suspicious array cast

suppress violation by javadoc tag

Return of 'null'

suppress: "we are not ready to migrate to java.util.Optional for now"

Duplicate Property

unselect option for values duplicates detection, this should resolve all violations. comment over option in config: "Different properties could have the same value, but could change at any time."

Inconsistent Resource Bundle

sould be resolved. Please use google translator to change text of message if required. Users will contribute to more valid wording, but this inspection will be in play already keep that rule.

Properties File or Resource Bundle is Alphabetically Unsorted

it is reasonable to fix this. All we case is to keep messages for the same Check close to each other, but we name(should name) properties by Check prefix so only location of such groups will change - it is ok.
Please use unix shell commands to do that sorting or (LibreOffice calc can do this, or ....)

Member

romani commented Jul 16, 2017

Non-final field referenced in 'equals()'
Non-final field referenced in 'hashCode()'

for SuppressElement fix by creation of c-tor that takes all required parameters, all fields should be final.
SuppressionFilter - use javadoc tag suppression, will be investigated at #4734 .

Object comparison using '==', instead of 'equals()'

suppress: "we like comparison of DetailAst by ==, probably one day we will be punished for this"

Suspicious array cast

suppress violation by javadoc tag

Return of 'null'

suppress: "we are not ready to migrate to java.util.Optional for now"

Duplicate Property

unselect option for values duplicates detection, this should resolve all violations. comment over option in config: "Different properties could have the same value, but could change at any time."

Inconsistent Resource Bundle

sould be resolved. Please use google translator to change text of message if required. Users will contribute to more valid wording, but this inspection will be in play already keep that rule.

Properties File or Resource Bundle is Alphabetically Unsorted

it is reasonable to fix this. All we case is to keep messages for the same Check close to each other, but we name(should name) properties by Check prefix so only location of such groups will change - it is ok.
Please use unix shell commands to do that sorting or (LibreOffice calc can do this, or ....)

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 17, 2017

Contributor

@NotNull/@Nullable problems
Ok to add annotation.

@romani when I add @NotNull annotation to the parameter it doesn't fix violation, @Nonnull fixes it but it fires checkstyle violation.

Contributor

Nimfadora commented Jul 17, 2017

@NotNull/@Nullable problems
Ok to add annotation.

@romani when I add @NotNull annotation to the parameter it doesn't fix violation, @Nonnull fixes it but it fires checkstyle violation.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 17, 2017

Member

fixes it but it fires checkstyle violation.

please always share details. What violations are raised by checkstyle?

Member

romani commented Jul 17, 2017

fixes it but it fires checkstyle violation.

please always share details. What violations are raised by checkstyle?

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 17, 2017

Contributor

@romani @Nonnull annotation is forbidden

Contributor

Nimfadora commented Jul 17, 2017

@romani @Nonnull annotation is forbidden

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 17, 2017

Member

annotation is forbidden

details is output from log with all information that build give to you.
After some googling, This annotation is not in java yet - https://gualtierotesta.wordpress.com/2013/04/30/not-null-arguments-in-java-public-methods/
we need to suppress this inspection: "we are not ready to use extra dependency com.google.code.findbugs:jsr305 in our API"

Member

romani commented Jul 17, 2017

annotation is forbidden

details is output from log with all information that build give to you.
After some googling, This annotation is not in java yet - https://gualtierotesta.wordpress.com/2013/04/30/not-null-arguments-in-java-public-methods/
we need to suppress this inspection: "we are not ready to use extra dependency com.google.code.findbugs:jsr305 in our API"

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 20, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 20, 2017

Nimfadora added a commit to Nimfadora/checkstyle that referenced this issue Jul 21, 2017

romani added a commit that referenced this issue Jul 21, 2017

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 22, 2017

Contributor

@romani

StringBuffer or StringBuilder without initial capacity

please recheck pitest reaction on this, it is better to have initial values, but if pitest makes mutations that we can not resolve, we need to suppress this inspection.

I've taken closer look at those cases and I'm not sure what exactly value should I place instead of default? Each time stringbuffer fills dynamically, baseda on method input of class state.

Contributor

Nimfadora commented Jul 22, 2017

@romani

StringBuffer or StringBuilder without initial capacity

please recheck pitest reaction on this, it is better to have initial values, but if pitest makes mutations that we can not resolve, we need to suppress this inspection.

I've taken closer look at those cases and I'm not sure what exactly value should I place instead of default? Each time stringbuffer fills dynamically, baseda on method input of class state.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 22, 2017

Member

@Nimfadora , please confirm that pitest will not make problems for us.
if no problem - I will provide numbers.

Member

romani commented Jul 22, 2017

@Nimfadora , please confirm that pitest will not make problems for us.
if no problem - I will provide numbers.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 22, 2017

Contributor

@romani pitest seem to be ok - I've added capacity to one of string builders and pitest was ok with it.

Contributor

Nimfadora commented Jul 22, 2017

@romani pitest seem to be ok - I've added capacity to one of string builders and pitest was ok with it.

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 23, 2017

Contributor

@romani

Duplicated Code

In the ScopeUtils there are two identical methods - isInAnnotationBlock and isInInterfaceBlock. Should I remove one of them?

Contributor

Nimfadora commented Jul 23, 2017

@romani

Duplicated Code

In the ScopeUtils there are two identical methods - isInAnnotationBlock and isInInterfaceBlock. Should I remove one of them?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 23, 2017

Member

Should I remove one of them?

yes, please make util method isInBlockOf(TokenType), current diff is:
image

Member

romani commented Jul 23, 2017

Should I remove one of them?

yes, please make util method isInBlockOf(TokenType), current diff is:
image

@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 23, 2017

Contributor

@romani after PR merge task may be closed

Contributor

Nimfadora commented Jul 23, 2017

@romani after PR merge task may be closed

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jul 24, 2017

Nimfadora pushed a commit to Nimfadora/checkstyle that referenced this issue Jul 24, 2017

romani added a commit that referenced this issue Jul 24, 2017

@romani romani added this to the 8.1 milestone Jul 24, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 24, 2017

Member

fixes are merged.

Member

romani commented Jul 24, 2017

fixes are merged.

@romani romani closed this Jul 24, 2017

@romani romani moved this from In Progress to Done in Practice What You Preach Jul 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment