Issue #4714: Make SuppressionCommentFilter and SuppressWithNearbyCommentFilter children of TreeWalker #4755

timurt · 2017-07-18T08:08:33Z

codecov-io · 2017-07-18T14:49:58Z

Codecov Report

Merging #4755 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4755   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         287     288    +1     
  Lines       15396   15432   +36     
  Branches     3499    3504    +5     
======================================
+ Hits        15396   15432   +36

Impacted Files	Coverage Δ
...s/checkstyle/filters/SuppressionCommentFilter.java	`100% <100%> (ø)`	⬆️
...pycrawl/tools/checkstyle/TreeWalkerAuditEvent.java	`100% <100%> (ø)`
...va/com/puppycrawl/tools/checkstyle/TreeWalker.java	`100% <100%> (ø)`	⬆️
...style/filters/SuppressWithNearbyCommentFilter.java	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c10bc5...d365551. Read the comment docs.

rnveach · 2017-07-18T15:27:09Z

@MEZk @timurt @romani

From wercker.

project postgresql: Failed during checkstyle configuration: SuppressionCommentFilter is not allowed as a child in Checker -> [Help 1]

Doing this in one change is going to be hard, and probably impossible thanks to wercker and all the regression we do. This change can't be made to other projects until we release this newest code.

@MEZk I recommend we split this PR into 2.
1st PR: The introduction of AstFilter and TreeWalkerAuditEvent.
2nd PR: The rewrite of SuppressionCommentFilter and SuppressWithNearbyCommentFilter to use the new filtering and events.
The delay of the second PR will atleast allow us to validate the rest of the changes in wercker.
This sound good?

@romani To keep this class backwards compatible should we have them implement Filter too, or just duplicate the entire class with different Filters? Duplication might not be possibly because of other breaking changing that this change probably wants.

rnveach

ModuleReflectionUtils needs to be updated to recognize the new filters.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/utils/ModuleReflectionUtils.java

Line 75 in ddeb553

|| isFilterModule(clazz)

MEZk · 2017-07-18T16:09:09Z

@rnveach

This change can't be made to other projects until we release this newest code.

Yes, this PR will completely break compatibility with previous versions. @romani This is what we intend to do.

To keep this class backwards compatible should we have them implement Filter too

No, we decided to break compatibility.

Doing this in one change is going to be hard, and probably impossible thanks to wercker and all the regression we do.

Yes, but we should do this. @romani As far as I remember we decided not to split this PR.

The delay of the second PR will atleast allow us to validate the rest of the changes in wercker.
This sound good?

Why do we need to introduce just interface and a wrapper (TreeWalkerAuditEvent) ? What for? They do not contain logic.

rnveach · 2017-07-18T16:55:06Z

Why do we need to introduce just interface and a wrapper

I meant all logic too except for backward breaking stuff.

we decided to break compatibility.

I will make a suggestion in gitter on this. If we are breaking, we might as well disable all of wercker.

romani · 2017-07-18T20:12:24Z

We could make transition without braking compatibility:

introduce new interfaces, classes
copy existing filters to new filters. So there will be 4 filters , 2 for Checker and 2 for TreeWalker
release filters for TreeWalker
migrate project that we use in CI
update our CI to use latest commit for projects
remove old filters.
introduce braking compatibility for all community.

Disadvantage: but we will end up with new Filter names and there still CI related changes but smooth.

Braking compatibility:

we keep names and brake compatibility on a commit, CI will react on it.
we can disable wercker for 1-2 weeks OR we can make forks+fix for all project in Wercker OR we do file substitution by wget in clones sources for they checkstyle config (this could be done in the same PR so CI will always be green).
CI come back to green if required
after release, help to upgrade project from CI to new chekstyle and new config.
after upgrade is done, switch to most recent commit and remove hack(fork/wget) from CI

Good: names are the same,CI is always green.
Bad: we will rely on some personal gist/hosting to keep updated files.

I do prefer to follow "Braking compatibility" as it will speedup acceptance of fixes and will keep names and will be the same efforts on CI update.

romani · 2017-07-18T21:04:40Z

@timurt , while we try to persuade @rnveach , please prepare separate commit in this PR to substitute all configs in affected projects with file from your gist.
You can use "patch approach", keep in gist only patch(not whole file content) if you are good with patch shell tool.
All wercker commands you can run on your local, they are just shell commands.

rnveach · 2017-07-18T23:28:49Z

while we try to persuade @rnveach

@romani If you are referring to breaking compatibility I am fine with that. I assumed we would want to try to avoid breaking compatibility like we have in other PRs.
If you are referring to any other chats in gitter, I have made my case and you can proceed with how you wish to keep wercker working.

MEZk · 2017-07-19T13:44:57Z

@timurt
Please, rebase your branch and make changes suggested by @romani to fix wercker build.

rnveach · 2017-07-20T02:34:58Z

@romani

with file from your gist.

Why aren't we doing it from a location all of us can access? If we restrict access to one person, it will cause a conflict if they happen to go away when an emergency fix is needed.

MEZk · 2017-07-20T05:23:12Z

@timurt
Ping

romani · 2017-07-20T19:53:10Z

Why aren't we doing it from a location all of us can access?

just try to economize time, as release is planned to this weekend, so that location is very temporal.
We could have shared gist at https://gist.github.com/checkstyle
@timurt , can fork our contribution repo and make PR it with patch files, we will not merge this PR, it will allow him to use that files for wget.

MEZk · 2017-07-20T20:32:35Z

@timurt
Do you still have problems with Wercker CI? Do you understand what you should do after my private message?

timurt · 2017-07-21T10:49:03Z

@MEZk @romani @rnveach
I changed checkstyle_sevntu_checks.xml

I made separate commit for wecker.yml containing gist patches
I've tested locally using bash commands, it worked fine
Can you check, is it ok?

rnveach · 2017-07-21T11:18:13Z

in wercker: Postgresql JDBC Driver failed with run.sh: line 9: patch: command not found.

rnveach · 2017-07-21T11:20:06Z

config/checkstyle_checks.xml

@@ -155,6 +141,20 @@
             value="The warning ''{0}'' cannot be suppressed at this location. Only few javac warnings are allowed to suppress. If try to suppress checkstyle/pmd/..... violation please do this in their config file. If you try to suppress IntelliJ IDEA inspection, please use javadoc block tag @noinspection"
             />
    </module>
+    <module name="SuppressionCommentFilter">


These are filters not annotations as the header on line 122 suggest.
Make a new header for filters inside TreeWalker and move these lines right under it.

Please also make sure all headers are in alphabetical order.

rnveach · 2017-07-21T11:20:45Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

                    final DetailAST rootAST = parse(contents);

                    if (!ordinaryChecks.isEmpty()) {
                        walk(rootAST, contents, AstState.ORDINARY);
                    }
                    if (!commentChecks.isEmpty()) {
                        final DetailAST astWithComments = appendHiddenCommentNodes(rootAST);
-


restore empty line.

timurt · 2017-07-21T11:38:14Z

@rnveach
run.sh: line 9: patch: command not found - how patches should be applied without patch command? Or should I insert apt install inside wecker.yml?

rnveach · 2017-07-21T11:45:36Z

how patches should be applied without patch command?

We should have access to the git command since it is pulling in our work from it.
My original plan was to use git to apply the patch.

romani · 2017-07-21T13:10:27Z

@timurt ,
please use git to apply - https://www.devroom.io/2009/10/26/how-to-create-and-apply-a-patch-with-git/
if any problem again .... please use whole file override.
We have few days only before release day (this Sunday), we need to merge you as soon as possible.

romani

item to improve:

romani · 2017-07-21T13:14:33Z

wercker.yml

@@ -46,7 +46,10 @@ build:
          CS_POM_VERSION=$(mvn -q -Dexec.executable='echo' -Dexec.args='${project.version}' --non-recursive org.codehaus.mojo:exec-maven-plugin:1.3.1:exec)
          echo CS_version: ${CS_POM_VERSION}
          for i in 1 2 3 4 5; do git clone https://github.com/pgjdbc/pgjdbc.git && break || sleep 15; done
-          cd pgjdbc/pgjdbc
+          cd pgjdbc/pgjdbc/src/main/checkstyle


please avoid change of this line cd ...., HACK lines(wget+patch) should not affect other lines to be easily removed.

romani

more items to change

romani · 2017-07-21T13:26:32Z

src/main/java/com/puppycrawl/tools/checkstyle/api/AstFilter.java

+ *
+ * @author Timur Tibeyev.
+ */
+public interface AstFilter {


@timurt , @MEZk
I do not remember that I allowed new classes in API.
We discussed to keep then out of API till we finish implementation, to make sure that tested that contract and we could keep it.
One more point is names of interfaces: AstFilter vs TreeWalkerAuditEvent .
it is better to name filter as TreeWalkerFilter.

@romani

AstFilter should be outside API package.

If somebody implements the check which is able to work with AST like TreeWalker, the filter will be available to use in that check. Why should we name it TreeWalker* ?

both names should consistent as they can not be used without each other.
both interfaces should be out of API
two ways:

AstFilter AstAuditEvent

or

TreeWalkerFilter TreeWalkerAuditEvent

@timurt , @soon , @Vladlis , @rnveach , please share your vote.

If I have to choose between the 2, I currently like TreeWalker* better since it is connected with that module and Java compiling.
I dislike Ast slightly as Javadoc is a form of AST even though we call it Node. If Java upgrades to ANTLR4, will we rename these classes accordingly too and break previous compatibility? And how will we name them differently so not to confuse with Javadoc as both will be nodes?

agree with rnveach.
additional point is that TreeWalkerAuditEvent will contain not only Ast , but some other context from TreeWalker.

AstFilter AstAuditEvent

Maybe we should call them

CheckerFilter CheckerAuditEvent

because they are children of module Checker, similarly to TreeWalkerFilter

because they are children of module Checker, similarly to TreeWalkerFilter

it is already too late, with such a change you will brake whole word of Checkstyle.

I would also vote for TreeWalkerAuditEvent since it is produced by TreeWalker and, therefore, for TreeWalkerFilter

I dislike Ast slightly as Javadoc is a form of AST even though we call it Node. If Java upgrades to ANTLR4, will we rename these classes accordingly too and break previous compatibility? And how will we name them differently so not to confuse with Javadoc as both will be nodes?

Ok, lets name them as TreeWalkerFilter and TreeWalkerAuditEvent.

@timurt
Please, do.

rnveach

We removed FileContentsHolder from filters but documentation in xdoc still makes reference to FileContentsHolder.

See http://checkstyle.sourceforge.net/config_filters.html#SuppressWithNearbyCommentFilter and http://checkstyle.sourceforge.net/config_filters.html#SuppressionCommentFilter in Description and Examples.

XDoc needs to be updated for all affected filters.

rnveach · 2017-07-22T19:56:00Z

wercker.yml

-          mvn -s settings-example.xml clean install -DskipTests=true -Dtest.elasticsearch.host.provided=true
+          wget https://gist.githubusercontent.com/timurt/f8e6c08b681337dfee76fdf8cc4ebb37/raw/c95333027111d055de8c943d3bc0b757ad638605/hibernate.patch
+          git apply hibernate.patch
+          mvn -s settings-example.xml clean install -DskipTests=true -Dtest.elasticsearch.host.provided=true -Dpuppycrawl.checkstyle.version=${CS_POM_VERSION}


@romani Looks like we had a bug with hibernate

@rnveach , what do you mean by this ?

MEZk · 2017-07-22T19:55:11Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

    @Override
    public void setupChild(Configuration childConf)
            throws CheckstyleException {
        final String name = childConf.getName();
        final Object module = moduleFactory.createModule(name);
-        if (!(module instanceof AbstractCheck)) {
+        if (module instanceof AbstractCheck) {
+            final AutomaticBean bean = (AutomaticBean) module;


@timurt
Why do we need to treat AbstractCheck as AutomaticBean directly? Please, restore original actions:

final AbstractCheck check = (AbstractCheck) module; check.contextualize(childContext); check.configure(childConf); check.init(); registerCheck(check);

Done, but again this TC violation

175: setupChild() Cast (AutomaticBean) module conflicts with surrounding 'instanceof' check

Should I suppress it?

resolved, TeamCity passed

MEZk · 2017-07-22T19:58:30Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

-                "TreeWalker is not allowed as a parent of " + name
-                        + " Please review 'Parent Module' section for this Check in web"
-                        + " documentation if Check is standard.");
+                    "TreeWalker is not allowed as a parent of " + name


@timurt
Do not ignore this point

MEZk · 2017-07-22T19:59:56Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

@@ -169,10 +188,11 @@ protected void processFiltered(File file, FileText fileText) throws CheckstyleEx
        if (CommonUtils.matchesFileExtension(file, getFileExtensions())) {
            final String msg = "%s occurred during the analysis of file %s.";
            final String fileName = file.getPath();
+            final FileContents contents = new FileContents(fileText);
+
            try {
                if (!ordinaryChecks.isEmpty()


@timurt
So, FileContext holder position should be restored. It should be closer to its first usage.

MEZk · 2017-07-22T20:02:05Z

src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java

+        }
+
+        final String[] expected = {
+            "2:16: Name 'I' must match pattern '^[a-z][a-zA-Z0-9]*$'.",


Use getCheckMessage, do not hardcode violation message in the test.

MEZk · 2017-07-22T20:05:00Z

@timurt
CIs should be fixed. Please, use mvn clean verify before sending your changes.

MEZk · 2017-07-23T04:50:40Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalkerFilter.java

+ *
+ * @author Timur Tibeyev.
+ */
+@FunctionalInterface


You declare functional interface. but do not use lambdas. Please, remove. It is not necessary. Future contributor will not understand your intention.

@MEZk
Done
Added noinspection javadoc because TeamCity was failing with violation:

27: TreeWalkerFilter Interface TreeWalkerFilter may be annotated with @FunctionalInterface

@rnveach
Please explain why do we need the annotation ?

annotation is allowed here: #3435 (comment)

rnveach · 2017-07-23T11:09:48Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalkerFilter.java

+ * An interface for filtering {@code TreeWalkerAuditEvent}.
+ *
+ * @author Timur Tibeyev.
+ * @noinspection InterfaceMayBeAnnotatedFunctional


Taken from #4755 (comment) ,
remove suppression and add annotation.

Please explain why do we need the annotation ?

@MEZk See issue #3435 .
We added annotation to all interfaces and did no suppressions.

rnveach · 2017-07-23T11:12:12Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

@@ -144,23 +155,35 @@ public void finishLocalSetup() {
        childContext = checkContext;
    }

+    /**
+     * {@inheritDoc} Creates child module.
+     * @noinspection ChainOfInstanceofChecks


Why do we need this suppression here when Checker, which has similar code doesn't need it?
https://github.com/rnveach/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L436

@rnveach
Your example from your forked repo, inside main repo this suppression exists
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L442

Otherwise TeamCity fails

Sorry, I used the wrong link to my own fork.
You are correct and can ignore this.

rnveach · 2017-07-23T11:13:00Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

    @Override
    public void setupChild(Configuration childConf)
            throws CheckstyleException {
        final String name = childConf.getName();
        final Object module = moduleFactory.createModule(name);
-        if (!(module instanceof AbstractCheck)) {
+        if (module instanceof AutomaticBean) {


Should this be surrounded by a try/catch like Checker?
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L450-L462

@rnveach
I think moduleFactory.createModule(name); is where exception probably could happen, but i was not surrounded by try/catch before. Should I do it now?

It was a question to more to the others. It may be helpful to let the user know their module cant initialize versus some other exception that can happen around there.
@MEZk @romani

createModule already put in exception all required context for user - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java#L183

but other methods contextualize and configure will not provide context of which module was in reflection initialization process.
@timurt , please add try-catch.

UPDATE: moved to #4814

@romani @rnveach @MEZk
As I understood, TreeWalker is the child of the Checker. So if any exception occurs inside TreeWalker.setupChild it will be caught inside Checker.setupChild, right?
Is this try-catch necessary?
It will require some tests to be changed, because new thrown exception differs from old one.

Previous exception message

cannot initialize module com.puppycrawl.tools.checkstyle.TreeWalker - Cannot set property 'excludedPackages' to 'com.puppycrawl.tools.checkstyle.checks.metrics.inputs.a.' in module com.puppycrawl.tools.checkstyle.checks.metrics.ClassFanOutComplexityCheck

Exception with try/catch inside TreeWalker.setupChild

cannot initialize module com.puppycrawl.tools.checkstyle.checks.metrics.ClassFanOutComplexityCheck - Cannot set property 'excludedPackages' to 'com.puppycrawl.tools.checkstyle.checks.metrics.inputs.a.' in module com.puppycrawl.tools.checkstyle.checks.metrics.ClassFanOutComplexityCheck

Other moment, if for example TreeWalker.setupChild catches exception and throws CheckstyleException. This thrown exception will be caught by Checker.setupChild and message becomes little bit weird:

cannot initialize module com.puppycrawl.tools.checkstyle.TreeWalker - cannot initialize module com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck - Cannot set property 'option' to 'invalid_option' in module com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck

original message

cannot initialize module com.puppycrawl.tools.checkstyle.TreeWalker - Cannot set property 'option' to 'invalid_option' in module com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck

lets move this nuance to #4814 .
It could be done later.

@rnveach , @MEZk , please approve PR.

rnveach · 2017-07-23T11:15:00Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

@@ -59,6 +63,7 @@
 *
 * @author Oliver Burn
 */
+// -@cs[ClassFanOutComplexity] Number of classes current class relies on exceeds 25.


Suppression comment should be on why you can't comply with check, not what violation was.
Example: https://github.com/checkstyle/checkstyle/blob/master/config/suppressions.xml#L74-L75

@rnveach
Please explain why do we need the annotation ?

romani

@rnveach , @MEZk if you are agree with changes please do not forget to click "Approve changes" in review. I will do merge as soon as all of us approve changes.

one point from me (moved to #4814 ):

romani · 2017-07-23T15:36:07Z

src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java

    @Override
    public void setupChild(Configuration childConf)
            throws CheckstyleException {
        final String name = childConf.getName();
        final Object module = moduleFactory.createModule(name);
-        if (!(module instanceof AbstractCheck)) {
+        if (module instanceof AutomaticBean) {


createModule already put in exception all required context for user - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java#L183

but other methods contextualize and configure will not provide context of which module was in reflection initialization process.
@timurt , please add try-catch.

UPDATE: moved to #4814

rnveach

I don't see anything else.

rnveach · 2017-07-24T12:59:09Z

src/test/java/com/puppycrawl/tools/checkstyle/TreeWalkerTest.java

+        final File file = temporaryFolder.newFile("file.java");
+        try (Writer writer = new BufferedWriter(
+                new OutputStreamWriter(new FileOutputStream(file), StandardCharsets.UTF_8))) {
+            final String content = "public class Main {\n"


Why isn't this in an input file? Why do we have to create it at runtime?

I thought source is too small to be stored inside separate file

Size doesn't matter for inputs. Please move this to input file.
Code and inputs should be separated. Inputs are in files so they can be verified that they compile fine. We don't deal with non-compilable files.

rnveach · 2017-07-24T13:03:50Z

@romani Looks like we had a bug with hibernate

@rnveach , what do you mean by this ?

Look at change in command.
Previous ended with -Dtest.elasticsearch.host.provided=true.
New command ends with -Dtest.elasticsearch.host.provided=true -Dpuppycrawl.checkstyle.version=${CS_POM_VERSION}.
It looks like it was missing the forcing of the snapshot checkstyle version, so we were probably never really testing regression.
Right?

Edit: I see we have snapshot in the next command, which actually runs regression. The line commented on was building their custom modules, I assume? so we were missing regression with that.

timurt · 2017-07-24T13:26:42Z

@rnveach

It looks like it was missing the forcing of the snapshot checkstyle version, so we were probably never really testing regression.

I think no, inside NoErrorTest - Hibernate Search we have two maven commands. I am not sure what they do

mvn -s settings-example.xml clean install -DskipTests=true -Dtest.elasticsearch.host.provided=true

Probably runs hibernate-search/build-config/src/main/resources/checkstyle.xml for build-config directory and copies this checkstyle.xml to subprojects.

mvn -s settings-example.xml checkstyle:check -Dpuppycrawl.checkstyle.version=${CS_POM_VERSION}

Probably runs checkstyle for all subprojects. So it would definitely fail, if compatibility error occurred.

I added -Dpuppycrawl.checkstyle.version=${CS_POM_VERSION} to the first command, because otherwise it was failing with error like:

SuppressionCommentFilter cannot be child of the TreeWalker

timurt · 2017-07-24T13:35:37Z

@rnveach @romani
Also Hibernate Search have two source file which are not compatible with last versions of checkstyle. Inside DoubleSpacesCheckTest.java and StringSuppressorTest source files FileText.fromLines is used, which was replaced by constructor FileText(File file, List<String> lines).
I had this error on my local machine and I wonder why wercker/build was not failing before this PR

…NearbyCommentFilter children of TreeWalker

…hNearbyCommentFilter gist patches

MEZk · 2017-07-24T17:57:38Z

@romani
Are we ready to merge?

MEZk self-assigned this Jul 18, 2017

timurt force-pushed the 4714 branch from 703bffe to 57bd344 Compare July 18, 2017 14:49

rnveach requested changes Jul 18, 2017

View reviewed changes

rnveach mentioned this pull request Jul 20, 2017

Issue #4727: fixed FinalLocalVariable scanning anonymous classes #4736

Merged

timurt force-pushed the 4714 branch from 57bd344 to fbd55d0 Compare July 20, 2017 11:07

timurt force-pushed the 4714 branch from fbd55d0 to 7a206db Compare July 21, 2017 10:45

timurt force-pushed the 4714 branch from 7a206db to 58ed859 Compare July 21, 2017 11:08

rnveach requested changes Jul 21, 2017

View reviewed changes

romani requested changes Jul 21, 2017

View reviewed changes

timurt force-pushed the 4714 branch 2 times, most recently from 9082055 to 6ff7c2c Compare July 21, 2017 17:29

rnveach requested changes Jul 21, 2017

View reviewed changes

timurt force-pushed the 4714 branch from a67ab31 to e42da8b Compare July 22, 2017 19:47

rnveach reviewed Jul 22, 2017

View reviewed changes

MEZk suggested changes Jul 22, 2017

View reviewed changes

timurt force-pushed the 4714 branch 2 times, most recently from 9735983 to ea45b23 Compare July 22, 2017 21:50

MEZk suggested changes Jul 23, 2017

View reviewed changes

timurt force-pushed the 4714 branch 4 times, most recently from a469677 to 4fad17a Compare July 23, 2017 07:28

rnveach requested changes Jul 23, 2017

View reviewed changes

rnveach reviewed Jul 23, 2017

View reviewed changes

timurt force-pushed the 4714 branch from 4fad17a to e3aa0a8 Compare July 23, 2017 13:08

romani requested changes Jul 23, 2017

View reviewed changes

romani mentioned this pull request Jul 24, 2017

review TreeWalker.setupChild method for exceptional cases #4814

Closed

romani approved these changes Jul 24, 2017

View reviewed changes

MEZk approved these changes Jul 24, 2017

View reviewed changes

rnveach requested changes Jul 24, 2017

View reviewed changes

timurt added 2 commits July 24, 2017 20:12

Issue checkstyle#4714: Make SuppressionCommentFilter and SuppressWith…

63bf86f

…NearbyCommentFilter children of TreeWalker

Issue checkstyle#4714: Apply SuppressionCommentFilter and SuppressWit…

d365551

…hNearbyCommentFilter gist patches

timurt force-pushed the 4714 branch from e3aa0a8 to d365551 Compare July 24, 2017 14:12

rnveach approved these changes Jul 24, 2017

View reviewed changes

romani merged commit 1695cab into checkstyle:master Jul 24, 2017

rnveach mentioned this pull request Jul 31, 2017

config: remove skipping sevntu checks #4860

Merged

romani added this to Done in Flexible Suppression Model Apr 15, 2018

Issue #4714: Make SuppressionCommentFilter and SuppressWithNearbyCommentFilter children of TreeWalker #4755

Issue #4714: Make SuppressionCommentFilter and SuppressWithNearbyCommentFilter children of TreeWalker #4755

Conversation

timurt commented Jul 18, 2017

codecov-io commented Jul 18, 2017 • edited

Codecov Report

rnveach commented Jul 18, 2017 • edited

rnveach left a comment

Choose a reason for hiding this comment

MEZk commented Jul 18, 2017

rnveach commented Jul 18, 2017

romani commented Jul 18, 2017

romani commented Jul 18, 2017

rnveach commented Jul 18, 2017

MEZk commented Jul 19, 2017

rnveach commented Jul 20, 2017 • edited

MEZk commented Jul 20, 2017

romani commented Jul 20, 2017

MEZk commented Jul 20, 2017

timurt commented Jul 21, 2017 • edited

rnveach commented Jul 21, 2017 • edited

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

timurt commented Jul 21, 2017

rnveach commented Jul 21, 2017 • edited

romani commented Jul 21, 2017

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

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

rnveach Jul 21, 2017 • edited

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 left a comment • edited

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

MEZk commented Jul 22, 2017

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

timurt Jul 23, 2017 • edited

Choose a reason for hiding this comment

rnveach Jul 23, 2017 • edited

Choose a reason for hiding this comment

rnveach Jul 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani Jul 23, 2017 • edited

Choose a reason for hiding this comment

timurt Jul 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 18, 2017 •

edited

rnveach commented Jul 18, 2017 •

edited

rnveach commented Jul 20, 2017 •

edited

timurt commented Jul 21, 2017 •

edited

rnveach commented Jul 21, 2017 •

edited

rnveach commented Jul 21, 2017 •

edited

rnveach Jul 21, 2017 •

edited

rnveach left a comment •

edited

timurt Jul 23, 2017 •

edited

rnveach Jul 23, 2017 •

edited

rnveach Jul 23, 2017 •

edited

romani Jul 23, 2017 •

edited

timurt Jul 23, 2017 •

edited

romani left a comment •

edited

romani Jul 23, 2017 •

edited

rnveach Jul 24, 2017 •

edited

rnveach Jul 24, 2017 •

edited

rnveach commented Jul 24, 2017 •

edited

timurt commented Jul 24, 2017 •

edited

timurt commented Jul 24, 2017 •

edited

MEZk commented Jul 24, 2017 •

edited