Issue #461: FieldDeclarationsCountCheck (NEW) #480

vorburger · 2016-11-16T20:44:23Z

vorburger · 2016-11-16T20:49:13Z

@romani I've tried, but failed, to understand what it's trying to tell me with the following violation in a new class I introduced - the exact same import appears in many other *Check classes - what on earth is it complaining about that I'm importing wrong here - can you help?

[ERROR] src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldDeclarationsCountCheck.java:[22,1] (imports) ImportControl: Disallowed import - com.github.sevntu.checkstyle.Utils

rnveach · 2016-11-16T23:42:01Z

@vorburger That is a check that prevents certain packages from using what he has defined as illegal imports. It means you shouldn't be importing that class in FieldDeclarationsCountCheck.
The violation is referring to this line: 0f7539f#diff-1113b12ada701bc205c9c2de790bda2aR22
Currently only com.github.sevntu.checkstyle.checks.api package is allowed to use that import. I am unfamiliar with the reasoning for this. It could be an oversight for all I know.

It looks like you added new methods to Utils. Do they need to be there instead of inside FieldDeclarationsCountCheck?
Also, you'll need to rebase your class to remove the conflicts.

coveralls · 2016-11-17T11:06:04Z

Coverage decreased (-0.01%) to 98.205% when pulling 9dd41b0 on vorburger:issue461_FieldDeclarationsCountCheck into d77b072 on sevntu-checkstyle:master.

vorburger · 2016-11-17T11:08:20Z

@vorburger That is a check that prevents certain packages from using what he has defined as illegal imports.

http://checkstyle.sourceforge.net/config_imports.html#ImportControl

It means you shouldn't be importing that class in FieldDeclarationsCountCheck.
The violation is referring to this line: 0f7539f#diff-1113b12ada701bc205c9c2de790bda2aR22

Sure, I got that, but as I asked above, "the exact same import appears in many other *Check classes", see e.g. https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/EmptyPublicCtorInClassCheck.java#L7, so why is that allowed there but in my new class it's whining about it?

Currently only com.github.sevntu.checkstyle.checks.api package is allowed to use that import. I am unfamiliar with the reasoning for this. It could be an oversight for all I know.

There is no com.github.sevntu.checkstyle.checks.api package; thus proposing #484

It looks like you added new methods to Utils.

Yeah. I'm therefore proposing to solve this confounding "problem" by adding https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/480/files#diff-42857ffc420c808e64abbf2a0e0440b4R13

Do they need to be there instead of inside FieldDeclarationsCountCheck?

Yes, because I'll be re-using those util methods in other changes from #457 I'm about to propose, as soon as we can get this one agreed upon and merged.

An alternative to sharing those as static methods in that Utils class would be to introduce a new AbstractFieldCheck kinda class and put those there (as non-static). But given that there already is a Utils class, IMHO it's better to use that instead of class hierarchy for this purpose.

Also, you'll need to rebase your class to remove the conflicts.

Sure, done.

romani

please rebase and resolve all my comments.
Please reply as "done" (or "..whatever is a reply...") to EACH of it.

romani · 2016-11-22T00:41:37Z

sevntu-checks/src/main/java/com/github/sevntu/checkstyle/Utils.java

+    }
+
+    /**
+     * Check node for matching class, taken both FQN and short name into account.


what is FQN ?

OK, it's fully qualified name, I'll update the JavaDoc

romani · 2016-11-22T00:44:19Z

sevntu-checks/src/main/java/com/github/sevntu/checkstyle/Utils.java

+     * @param ast an AST node
+     * @return FQN of type
+     */
+    public static String getTypeName(final DetailAST ast) {


hosting in Utils imply no context.
So name of the method should be clear enough to not misuse. Does it work on any ast ?

OK, will rename it to getTypeNameOfFirstToken(), does that work better for you?

ok, for now

romani · 2016-11-22T00:46:30Z

sevntu-checks/src/main/java/com/github/sevntu/checkstyle/Utils.java

+     * @param fqnClassName
+     * @return true if type name of first token in node is fqnClassName, or its short name; false otherwise
+     */
+    public static boolean isMatchingClass(final DetailAST ast, final String fqnClassName) {


please make it as single point of exit from method.
Please rename it and make i like "matchesFullyQualifiedName"

OK, will be done in next push.

romani · 2016-11-22T00:48:53Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldDeclarationsCountCheck.java

+import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+
+/**
+ * Checks that classes have at most 1 field of the given className.


javadoc in sevntu project is all that users have to read about Check. Please extend description place examples of usage, see other checks for example.

romani · 2016-11-22T00:50:58Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldDeclarationsCountCheck.java

+    }
+
+    @Override
+    public int[] getDefaultTokens() {


please override all methods http://checkstyle.sourceforge.net/writingchecks.html#Understanding_token_sets
in most recent releases of checkstyle these methods become abstract.

OK, if you say so.. I've just implemented getRequiredTokens() and getAcceptableTokens() as return getDefaultTokens(). There is no setTokens(). If that is NOK, please update that documentation to clarify, or point me to an example in another existing *Check to follow?

I agree that our documentation is not ideal .... . Unfortunately usage of them is different for each Check, in some Check all of them return different in size array of Tokens (but always interchangeable).
Can you give us advices on how you would like too see this doc (in separate issue) ? For now I am blind as I spent too much time with that project.

romani · 2016-11-22T00:54:17Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldDeclarationsCountCheck.java

+    /**
+     * Field to hold previously seen Logger.
+     */
+    private String prevSeenClassName = "";


all fields of Checks that are not properties of Check, should be reset in beginTree method, to avoid causing cache problems on multi-file launch.

OK, thanks a lot for pointing this out; done in next iteration.

romani · 2016-11-22T00:57:52Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldDeclarationsCountCheck.java

+ * @author Milos Fabian, Pantheon Technologies - original author (in OpenDaylight)
+ * @author Michael Vorburger.ch - refactored and made more generalized for contribution to Sevntu
+ */
+public class FieldDeclarationsCountCheck extends AbstractCheck {


Check is named as "Count" , but no count could be adjusted.
Add property or rename a Check.

OK, renamed to FieldSingleDeclarationCheck in next iteration (also FieldSingleDeclarationCheckTest)

romani · 2016-11-22T00:59:40Z

...test/resources/com/github/sevntu/checkstyle/checks/coding/FieldDeclarationsCountCheckOK.java

+
+public class FieldDeclarationsCountCheckOK {
+
+    org.slf4j.Logger logger;


please make UT with class with two fields with full name.

OK, sure, if you insist.

romani · 2016-11-22T01:00:24Z

...est/resources/com/github/sevntu/checkstyle/checks/coding/FieldDeclarationsCountCheckNOK.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FieldDeclarationsCountCheckNOK {


please add examples of usage in classes that have subclasses with loggers.

NOK, because my understanding is that Checkstyle cannot handle this, because its based on analysing the AST of a single file; AFAIK I couldn't check parent class fields, right? So.. no point, I would have thought.

sorry, I meant inner classes. I did a comment one more time in my second iteration of review.

romani · 2016-11-22T01:04:36Z

...est/resources/com/github/sevntu/checkstyle/checks/coding/FieldDeclarationsCountCheckNOK.java

+public class FieldDeclarationsCountCheckNOK {
+
+    // both FQN and import'ed type names work:
+    Logger logger1;


please make a test case with "Logger" from different(not slf4j) package in imports.
there should be no violation.
Check have to process imports to know what short name is actually used, we have examples in other Checks for this.

NOK, I do not know how to do this.. can you point me to an existing Check with a corresponding Test that does this, so I can copy and learn from it? This seems a lower priority to me - perhaps something that could be dealt with in a follow-up, after this is accepted and merged?

Examples:

~/java/github/sevntu-checkstyle/sevntu.checkstyle/sevntu-checks/src/main/java [master|✔] $ grep -R "IMPORT" * | head -n 5 com/github/sevntu/checkstyle/checks/coding/ForbidInstantiationCheck.java: return new int[] {TokenTypes.IMPORT, TokenTypes.LITERAL_NEW }; com/github/sevntu/checkstyle/checks/coding/ForbidInstantiationCheck.java: case TokenTypes.IMPORT: com/github/sevntu/checkstyle/checks/coding/ForbidInstantiationCheck.java: * literal node ("IMPORT" or "LITERAL_NEW" node types). com/github/sevntu/checkstyle/checks/coding/ForbidInstantiationCheck.java: * "IMPORT" node or instanstiated class Name&Path for given com/github/sevntu/checkstyle/checks/coding/ForbidCertainImportsCheck.java: TokenTypes.IMPORT, TokenTypes.LITERAL_NEW, };

https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/ForbidInstantiationCheck.java

Yes, it could be postponed but in this case please mention this in javadoc as limitation that could be improved in future.

coveralls · 2016-11-22T17:23:32Z

Coverage decreased (-0.05%) to 98.234% when pulling 59d112a on vorburger:issue461_FieldDeclarationsCountCheck into 29b5ac0 on sevntu-checkstyle:master.

rnveach

@vorburger Please look into full file difference in checkstyle-extensions.xml. I think we should only see your additions.

rnveach · 2016-11-22T17:27:42Z

sevntu-checks/import-control.xml

@@ -10,6 +10,7 @@
  <allow pkg="com.google.common"/>
  <allow pkg="com.puppycrawl.tools.checkstyle.api"/>
  <allow pkg="com.puppycrawl.tools.checkstyle.checks"/>
+  <allow class="com.github.sevntu.checkstyle.Utils"/>


This was already fixed in previous PR, you can see it a few lines down from here. Please remove.

rnveach · 2016-11-22T17:33:26Z

sevntu-checks/src/main/java/com/github/sevntu/checkstyle/Utils.java

+        if (findFirstToken != null) {
+            final FullIdent ident = CheckUtils.createFullType(findFirstToken);
+            if (ident != null) {
+                return ident.getText();


single return point.

OK, done. (BTW: If this is important to you guys, how about activating Checkstyle rule for enforcing this?)

It is already an open issue in main project where sevntu get its configuration from.
Just no one has worked on it.

rnveach · 2016-11-22T17:39:24Z

sevntu-checks/src/main/java/com/github/sevntu/checkstyle/Utils.java

+    public static String getClassName(final DetailAST ast) {
+        DetailAST parent = ast.getParent();
+        while (parent.getType() != TokenTypes.CLASS_DEF
+                && parent.getType() != TokenTypes.ENUM_DEF) {


Since this is a common utility and your going up the chain, you have to be careful that you don't miss the top level class and create an NPE.
This looks like your missing INTERFACE_DEF and ANNOTATION_DEF.
It may not be pertinent to your check, but this is a common utility now.

OK, done; introduced a new isTypeDefinition() utility method

rnveach · 2016-11-22T17:45:04Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+    /**
+     * Field to hold previously seen Logger.
+     */
+    private String prevSeenClassName = "";


Why do we init with empty string and then set to null in begin tree and then back to empty string in finishTree? Let's be consistent.

OK, done, using empty string instead of null consistently everywhere now.

rnveach · 2016-11-22T17:46:56Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+    /**
+     * Class name check for.
+     */
+    private String className;


No default value means this check will issue an NPE if used without setting className.
@romani Should org.slf4j.Logger be the default value or should we have null protection?

@rnveach FYI I originally proposed this as a SingleLoggerCheck, and @romani objected saying that it had to be more general ... ;-) so I'll assume that he doesn't want org.slf4j.Logger to be the default value for className. I've added a null check in visitToken() now, with a IllegalStateException. Personally I like that much better than null check and just ignoring it - as a user, if I've configured this Check, I want to be clearly told (by a failure) why it doesn't do something, rather tha nit just silently not doing anything. PS: In an ideal world, I guess CS could have real concept of mandatory VS optional configuration properties... one day.

Ideally Check should be more general than default configuration.
If default configuration is not possible to make as reasonable, Check should do nothing and no crashes are expected.

rnveach · 2016-11-22T17:51:00Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+    }
+
+    @Override
+    public void finishTree(DetailAST rootAST) {


Since you do reset of fields in beginTree, this isn't needed. You really only need one or the other. beginTree is preferred so it will hit before check visit begins.

OK, removed.

rnveach · 2016-11-22T17:53:29Z

sevntu-checks/src/main/resources/com/github/sevntu/checkstyle/checks/coding/messages.properties

@@ -33,6 +33,7 @@ either.log.or.throw=Either log or throw exception.
 explicit.init=Variable ''{0}'' explicitly initialized to ''{1}'' (default value for its type).
 fall.through=Fall through from previous branch of the switch statement.
 fall.through.last=Fall through from the last branch of the switch statement.
+field.count={0} might be declared only once.


might? It describes 'possibility' of something. We already know it is declared multiple times, so we should be more direct with violations like should be.

OK, I think that was meant to be an imperative "may" instead of "might", but "should" is better; done.

rnveach · 2016-11-22T18:00:48Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+
+    @Override
+    public void visitToken(DetailAST ast) {
+        if (isVariable(ast) && Utils.matchesFullyQualifiedName(ast, className)) {


You are not looking at import statements. User can import class Logger and it not be from org.slf4j, but you would still mark it as violation if they didn't fully qualify it, right?
Why do I need to define full path in check, org.slf4j.Logger as your examples show, if check won't obey it? You could just give Logger only and it will be same result.
This seems similar to the check EitherLogOrThrowCheck.
@romani your thoughts?

@rnveach you're right, I think (from what I understand of CS). But this almost more of a limitation of CS.. I had a quick look at EitherLogOrThrowCheck, and see how that Check has code to deal with this (hasLoggerClassInImports), but... that almost looks a like a one off hack to me. If you would like Checks to be able to be written for both full and local type names, then shouldn't there be better shared infrastructure available for writing such Checks? I would argue that mixed types of Logger is extremely atypical, and so while I agree this isn't completely perfect strictly speaking, it seems like a very minor problem in real world, and we should ignore this for now, and move forward. @romani your thoughts?

shared infrastructure available for writing such Checks?

Yes we lack a lot on infrastructure util methods and examples and .... our team is very very tiny.
All we have is what you see in repo, all re welcome to improve :) .

it seems like a very minor problem in real world, and we should ignore this for now, and move forward.

we could, I am ok with multi-step development. All depends on your time.
But on each step logic should be clear for user. So if we agree on limitation, it should be in javadoc and user should clear see this, in other case there will be storm of requests/bugs from users.

rnveach · 2016-11-22T18:17:19Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+        if (isVariable(ast) && Utils.matchesFullyQualifiedName(ast, className)) {
+            final String nowVisitingClassName = Utils.getClassName(ast);
+            if (this.prevSeenClassName != null
+                    && this.prevSeenClassName.equals(nowVisitingClassName)) {


@vorburger This line puzzles me.
I am not sure when prevSeenClassName will not equal nowVisitingClassName. User gave specific class, so it should always be said class, right? I assume we are passing line/branch coverage here.
Can you point me to instance where equals evaluates to false?

OK, I've inherited this code from someone else. This is a left over from when it didn't reset in beginTree and finishTree; so it kept track of which class its visiting. Meanwhile, I guess this could be written differently now - could you please have a look at the latest iteration - makes sense?

New iteration definitely makes more sense to me.

vorburger · 2016-11-23T11:15:03Z

@rnveach re. "Please look into full file difference in checkstyle-extensions.xml. I think we should only see your additions.", this will be fixed by #490; unfortunately once again only one-off. In an ideal world, we really should do checkstyle/checkstyle#893 so we can do checkstyle/checkstyle#3557

vorburger · 2016-11-23T12:02:03Z

@romani could you please advise how to address the coverage problem shown below, which is due to the introduction of getAcceptableTokens() you've requested earlier above here.. I've no idea what that does, and don't see what one would have to do in the test to make that covered - can you point to a line in an existing test which leads to coverage of getAcceptableTokens() ? This is required because you seem to (now, new; since you've fixed Cobertura) be enforcing 100% test coverage.

[ERROR] com.github.sevntu.checkstyle.checks.coding.FieldSingleDeclarationCheck failed coverage check. Line coverage rate of 93.7% is below 100.0%

rnveach · 2016-11-23T12:35:19Z

how to address the coverage problem shown below, which is due to the introduction of getAcceptableTokens()

@vorburger
getAcceptableTokens is called in TreeWalker when it is first registering the check. It is only called when user specifies tokens and we have to verify if they should be allowed.
Every check accepts a property tokens. It is how we allow users to customize what some check validates pertaining to specific nodes. It is easier than specifying a bunch of boolean properties to turn things on and off.

Try adding something like config.addAttribute("tokens", "CLASS_DEF"); to a new test.
Edit: Might have to be VARIABLE_DEF, CLASS_DEF.

vorburger · 2016-11-23T13:41:55Z

@rnveach thank you, that seems to work; done.

coveralls · 2016-11-23T13:47:42Z

Coverage decreased (-0.2%) to 98.093% when pulling f6759e4 on vorburger:issue461_FieldDeclarationsCountCheck into 4f77fc6 on sevntu-checkstyle:master.

coveralls · 2016-11-25T10:10:42Z

Coverage decreased (-0.2%) to 98.093% when pulling 7991b1d on vorburger:issue461_FieldDeclarationsCountCheck into 12a68fd on sevntu-checkstyle:master.

Signed-off-by: Michael Vorburger <[email protected]>

coveralls · 2016-11-25T18:41:21Z

Coverage decreased (-0.2%) to 98.093% when pulling 9410b85 on vorburger:issue461_FieldDeclarationsCountCheck into b05ae88 on sevntu-checkstyle:master.

romani

Please resolve following items, please review https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/What-files-need-to-be-changed-or-created-in-repository-for-new-Check-implementation to provide all fixes for new Check.

romani · 2016-12-01T13:48:47Z

sevntu-checks/src/main/java/com/github/sevntu/checkstyle/Utils.java

+    }
+
+    /**
+     * Check node for matching class, taken both FQN and short name into account.


please put words for FQN, each static function is independent from each other , so no shared context/javadoc .

romani · 2016-12-01T13:50:18Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+    /**
+     * Field to remember if class was previously seen in current File.
+     */
+    private boolean hasPreviouslySeenClass;


base on javadoc it should be named isPreviouslySeenClass

romani · 2016-12-01T13:53:13Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+     * @param className the fully qualified name (FQN) of the class
+     */
+    public void setClassName(String className) {
+        this.fullyQualifiedClassName = className;


please name setter exactly as field is named (name of argument is ok), not sure why our Check (http://sevntu-checkstyle.github.io/sevntu.checkstyle/apidocs/com/github/sevntu/checkstyle/checks/coding/SimpleAccessorNameNotationCheck.html) did not catch that ....

@romani Sevntu does not run its own checks against itself. We only run Checkstyle checks.
See https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/pom.xml#L123

Yes.
So, please remove that annotation and catch with checking exact excetion and message of it.

romani · 2016-12-01T13:55:05Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+
+    @Override
+    public int[] getDefaultTokens() {
+        return new int[] {TokenTypes.VARIABLE_DEF };


checkstyle.sourceforge.net/writingchecks.html#Understanding_token_sets
please move that array return to getAcceptableTokens(), all other methods should call getAcceptableTokens().

romani · 2016-12-01T13:58:16Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+        if (fullyQualifiedClassName == null) {
+            throw new IllegalStateException("Must set mandatory className property in"
+                    + getClass().getSimpleName());
+        }


this is not good, please put some default value, it could match some class by default or to no class all like ""(empty string). In this case we will avoid null and make Check work even in misconfiguration.
In current implementation you will trow in IDE too much exceptions or in logs .... that is very inconvenient.

romani · 2016-12-01T14:00:29Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck.java

+                    + getClass().getSimpleName());
+        }
+        if (Utils.matchesFullyQualifiedName(ast, fullyQualifiedClassName)) {
+            if (hasPreviouslySeenClass) {


I do not see filtering for fields.
Your Check is named - FieldSingleDeclarationCheck , but you examine fields+variables.

romani · 2016-12-01T14:03:26Z

...rc/test/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheckTest.java

+        verify(config, getPath("FieldSingleDeclarationCheck_OK.java"), new String[] {});
+    }
+
+    @Test(expected = CheckstyleException.class)


@rnveach , I thought we already made a Check that control that bad practice.
sevntu-checkstyle.github.io/sevntu.checkstyle/apidocs/com/github/sevntu/checkstyle/checks/annotation/ForbidAnnotationElementValueCheck.html
Do we miss to add it to config ?

@romani See my comment above, sevntu doesn't run its own checks against itself. Only Checkstyle checks.
Plus, we don't run anything against Test in sevntu hence why there is no header in this file.

romani · 2016-12-01T14:05:35Z

...est/resources/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheck_OK.java

+
+    org.slf4j.Logger logger;
+
+}


I do not see tests for cases with inner classes (in class , in method, ..... yes real life code is crazy on purpose).
If we care only about Top level class we should define this in javadoc or .... .

rnveach

Minor changes from me.

Please see answers to old questions that are now hidden:
#480 (comment)
#480 (comment)

rnveach · 2016-12-01T14:25:02Z

...rc/test/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheckTest.java

+        String className = "org.slf4j.Logger";
+        config.addAttribute("className", className);
+        final String expected[] = { "10: " + getCheckMessage(MSG_KEY, className) };
+        verify(config, getPath("FieldSingleDeclarationCheck_NOK.java"), expected);


Input files should start with Input. I just started this on other files, and main repo already encourages it.

rnveach · 2016-12-01T14:26:35Z

...rc/test/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheckTest.java

+        config.addAttribute("className", className);
+        final String expected[] = { "10: " + getCheckMessage(MSG_KEY, className) };
+        verify(config, getPath("FieldSingleDeclarationCheck_NOK.java"), expected);
+        verify(config, getPath("FieldSingleDeclarationCheck_OK.java"), new String[] {});


One test per method please. It is weird seeing expected variable where it is only valid for one run and the other is expecting a different result.

rnveach · 2016-12-01T14:26:49Z

...rc/test/java/com/github/sevntu/checkstyle/checks/coding/FieldSingleDeclarationCheckTest.java

+        config.addAttribute("className", className);
+        final String expected[] = { "7: " + getCheckMessage(MSG_KEY, className) };
+        verify(config, getPath("FieldSingleDeclarationCheck_FQN_NOK.java"), expected);
+        verify(config, getPath("FieldSingleDeclarationCheck_OK.java"), new String[] {});


Same here, one test per method.

romani · 2016-12-02T01:19:16Z

@vorburger , please rebase and add your Check to https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/sevntu-checks.xml for testing over our code base by Travis.

romani · 2017-01-14T13:46:21Z

@vorburger , ping

romani · 2017-01-23T04:40:38Z

@vorburger , ping .
Please let me know if you want to drop this PR , smb else might finish your work

romani · 2017-04-21T10:32:19Z

@vorburger , ping .

romani · 2019-05-19T15:59:49Z

we lost connection to Author of this code, I am closing PR .... if author or any other person ready to continue, we can reopen PR or start it from scratch.

vorburger force-pushed the issue461_FieldDeclarationsCountCheck branch from 0f7539f to 9dd41b0 Compare November 17, 2016 10:55

romani requested changes Nov 22, 2016

View reviewed changes

vorburger force-pushed the issue461_FieldDeclarationsCountCheck branch 3 times, most recently from 7666545 to 59d112a Compare November 22, 2016 17:16

rnveach requested changes Nov 22, 2016

View reviewed changes

rnveach reviewed Nov 22, 2016

View reviewed changes

vorburger force-pushed the issue461_FieldDeclarationsCountCheck branch from 59d112a to 60519f6 Compare November 23, 2016 11:59

vorburger force-pushed the issue461_FieldDeclarationsCountCheck branch from 60519f6 to f6759e4 Compare November 23, 2016 13:41

vorburger force-pushed the issue461_FieldDeclarationsCountCheck branch from f6759e4 to 7991b1d Compare November 25, 2016 10:01

vorburger mentioned this pull request Nov 25, 2016

New Check: FieldMustBeConstantCheck - Logger must be declared as private static final #481

Open

Issue sevntu-checkstyle#461: FieldDeclarationsCountCheck (NEW)

9410b85

Signed-off-by: Michael Vorburger <[email protected]>

vorburger force-pushed the issue461_FieldDeclarationsCountCheck branch from 7991b1d to 9410b85 Compare November 25, 2016 18:36

romani requested changes Dec 1, 2016

View reviewed changes

rnveach requested changes Dec 1, 2016

View reviewed changes

romani added abandoned incomplete labels Jan 4, 2018

romani closed this May 19, 2019


		public class FieldDeclarationsCountCheckOK {

		org.slf4j.Logger logger;

Issue #461: FieldDeclarationsCountCheck (NEW) #480

Issue #461: FieldDeclarationsCountCheck (NEW) #480

Conversation

vorburger commented Nov 16, 2016 • edited Loading

vorburger commented Nov 16, 2016

rnveach commented Nov 16, 2016 • edited Loading

coveralls commented Nov 17, 2016

vorburger commented Nov 17, 2016

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vorburger Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vorburger Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 22, 2016

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vorburger commented Nov 23, 2016

vorburger commented Nov 23, 2016

rnveach commented Nov 23, 2016 • edited Loading

vorburger commented Nov 23, 2016

coveralls commented Nov 23, 2016

coveralls commented Nov 25, 2016

coveralls commented Nov 25, 2016

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vorburger commented Nov 16, 2016 •

edited

Loading

rnveach commented Nov 16, 2016 •

edited

Loading

vorburger Nov 22, 2016 •

edited

Loading

vorburger Nov 22, 2016 •

edited

Loading

rnveach Nov 22, 2016 •

edited

Loading

rnveach Nov 22, 2016 •

edited

Loading

rnveach commented Nov 23, 2016 •

edited

Loading

rnveach Dec 1, 2016 •

edited

Loading

rnveach Dec 1, 2016 •

edited

Loading