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: ninth part of idea violations #4707

Closed
Nimfadora opened this Issue Jul 12, 2017 · 6 comments

Comments

Projects
2 participants
@Nimfadora
Contributor

Nimfadora commented Jul 12, 2017

sub-task of #4681

too widespread to be included to the report:
Feature envy 961
Local variable of concrete class 3071
Method parameter of concrete class 1292
public method not exposed in interface 1335

report on all other violations

violations names included:

fixed:
CastToConcreteClass
InstanceVariableOfConcreteClass
InstanceofChain
LocalVariableOfConcreteClass
MagicNumber
MethodReturnOfConcreteClass
StaticMethodOnlyUsedInOneClass
StaticVariableOfConcreteClass
FeatureEnvy
InstanceofInterfaces
ParameterOfConcreteClass
PublicMethodNotExposedInInterface
TypeMayBeWeakened

will be addressed in another task:
BooleanParameter
@Nimfadora

This comment has been minimized.

Show comment
Hide comment
@Nimfadora

Nimfadora Jul 12, 2017

Contributor

TypeMayBeWeakened review may be skipped

Contributor

Nimfadora commented Jul 12, 2017

TypeMayBeWeakened review may be skipped

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 12, 2017

Member

CastToConcreteClass

suppress: "this is valid but there are many false-positvies as we use reflection to load modules after recheck by instanceof and we restricted in by existing api types, but some cases are valid, inspection lack of properties to make it less demanding"

InstanceofChain

suppress on test scope "we do reflection for testing "
suppress by javadoc annotation at Checker.setupChild

InstanceVariableOfConcreteClass

suppress: "this is valid but there are many false-positvies as we use reflection to load modules after recheck by instanceof"

MagicNumber

suppress in test scope.
suppress by javadoc annotation in MainFrame

MethodReturnOfConcreteClass

suppress: "it is too demanding, event violated private methods. Nothing is bad to be exact inside custom logic, might be good for api classes validation, but setup of this is too heavy for us"

BooleanParameter

make good sense.
please resolve on all cases except for ConfigurationLoader , DefaultLogger, XMLLogger.
ConfigurationLoader , DefaultLogger, XMLLogger result is big backward compatibility problem with plugins that we use in our CI, will be addressed at #4709, do suppression by javadoc annotation.
it should be resolved by defining public inner enum, to make it sure it will not have any meaning out of target class .
case of IllegalTokenTextCheck.setIgnoreCase()' we need to suppress as it is our API.
case of DetectorOptions , we need to suppress by javadoc annotation, it is bug that Builder pattern is not skipped.

StaticVariableOfConcreteClass

suppress: "it lacks option to skip inner field of the same size that we use give convenient ready to use instances"

StaticMethodOnlyUsedInOneClass

suppress: "we are a library, not used my us heavily does not mean that is not required"

LocalVariableOfConcreteClass

suppress : "we like concrete types in concrete logic, inspection is not practical and lack more options to skip cases"

Member

romani commented Jul 12, 2017

CastToConcreteClass

suppress: "this is valid but there are many false-positvies as we use reflection to load modules after recheck by instanceof and we restricted in by existing api types, but some cases are valid, inspection lack of properties to make it less demanding"

InstanceofChain

suppress on test scope "we do reflection for testing "
suppress by javadoc annotation at Checker.setupChild

InstanceVariableOfConcreteClass

suppress: "this is valid but there are many false-positvies as we use reflection to load modules after recheck by instanceof"

MagicNumber

suppress in test scope.
suppress by javadoc annotation in MainFrame

MethodReturnOfConcreteClass

suppress: "it is too demanding, event violated private methods. Nothing is bad to be exact inside custom logic, might be good for api classes validation, but setup of this is too heavy for us"

BooleanParameter

make good sense.
please resolve on all cases except for ConfigurationLoader , DefaultLogger, XMLLogger.
ConfigurationLoader , DefaultLogger, XMLLogger result is big backward compatibility problem with plugins that we use in our CI, will be addressed at #4709, do suppression by javadoc annotation.
it should be resolved by defining public inner enum, to make it sure it will not have any meaning out of target class .
case of IllegalTokenTextCheck.setIgnoreCase()' we need to suppress as it is our API.
case of DetectorOptions , we need to suppress by javadoc annotation, it is bug that Builder pattern is not skipped.

StaticVariableOfConcreteClass

suppress: "it lacks option to skip inner field of the same size that we use give convenient ready to use instances"

StaticMethodOnlyUsedInOneClass

suppress: "we are a library, not used my us heavily does not mean that is not required"

LocalVariableOfConcreteClass

suppress : "we like concrete types in concrete logic, inspection is not practical and lack more options to skip cases"

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

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

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

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 15, 2017

Member

Violations included:

please activate that list of inspections and send PR(not for merge) to generate report on TC.
After each merge to our master, update that PR with list of item that still unresolved, TC will keep report for you.

Member

romani commented Jul 15, 2017

Violations included:

please activate that list of inspections and send PR(not for merge) to generate report on TC.
After each merge to our master, update that PR with list of item that still unresolved, TC will keep report for you.

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

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

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 20, 2017

Member

I still do not see WIP pull request and report on this.
What is left unfixed ?

Member

romani commented Jul 20, 2017

I still do not see WIP pull request and report on this.
What is left unfixed ?

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

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

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

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

@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 25, 2017

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

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

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

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

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

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

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

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

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

@romani romani added this to the 8.2 milestone Aug 8, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 8, 2017

Member

fix is merged

Member

romani commented Aug 8, 2017

fix is merged

@romani romani closed this Aug 8, 2017

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

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