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

Violations should print ID and observe them as unique #4607

Closed
rnveach opened this Issue Jul 4, 2017 · 6 comments

Comments

3 participants
@rnveach
Member

rnveach commented Jul 4, 2017

$ cat TestClass.java
public class TestClass {
    void method() {
int test = 0;
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
<module name="FinalLocalVariable">
  <property name="id" value="1" />
</module>
<module name="FinalLocalVariable">
  <property name="id" value="2" />
</module>
    </module>
</module>

$ java -jar checkstyle-8.0-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:3:5: Variable 'test' should be declared final. [FinalLocalVariable]
Audit done.
Checkstyle ends with 1 errors.

Since I have 2 instances defined each with their own unique ID, I expect 2 violations defining which instance produced the violation.
Example:

[ERROR] TestClass.java:3:5: Variable 'test' should be declared final. [1]
[ERROR] TestClass.java:3:5: Variable 'test' should be declared final. [2]

XML output must be updated too to show the id.

This change is required for regression-tool to have multiple instances in one configuration. With all modules in 1 config, we should see less execution time then if we were using multiple configurations. We need a way to identify violations with the module instance that is defined.
patch-diff-report-tool will probably need changes to recognize this change too.

liscju added a commit to liscju/checkstyle that referenced this issue Jul 4, 2017

denizarsan added a commit to denizarsan/checkstyle that referenced this issue Jul 5, 2017

denizarsan added a commit to denizarsan/checkstyle that referenced this issue Jul 6, 2017

@checkstyle checkstyle deleted a comment from rnveach Jul 7, 2017

@checkstyle checkstyle deleted a comment from denizarsan Jul 7, 2017

@checkstyle checkstyle deleted a comment from rnveach Jul 7, 2017

@checkstyle checkstyle deleted a comment from denizarsan Jul 7, 2017

@checkstyle checkstyle deleted a comment from rnveach Jul 7, 2017

@checkstyle checkstyle deleted a comment from denizarsan Jul 7, 2017

@checkstyle checkstyle deleted a comment from rnveach Jul 7, 2017

@checkstyle checkstyle deleted a comment from rnveach Jul 7, 2017

@romani romani added the bug label Jul 7, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 7, 2017

Member

@Luolc , you are assigned for the issue by @rnveach looks like it make problem to you in your GSoC project.

Member

romani commented Jul 7, 2017

@Luolc , you are assigned for the issue by @rnveach looks like it make problem to you in your GSoC project.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 27, 2017

Member

Be aware of issue found at #3496 (comment) . I expect this fix to prevent problems like that happening again.

Member

rnveach commented Jul 27, 2017

Be aware of issue found at #3496 (comment) . I expect this fix to prevent problems like that happening again.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 31, 2017

Member

@Luolc Do you have issues starting this PR while you work in regression-tool?

Member

rnveach commented Jul 31, 2017

@Luolc Do you have issues starting this PR while you work in regression-tool?

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Jul 31, 2017

Contributor

@rnveach @romani
There are so many references of the getMessage method which used for printing the message. I have tried some ways to add the id, but all of the ways would affect UTs a lot. I spend a lot of time fixing UTs and keep rebasing the laster commit to avoid no new failure. Also I have reverted my work some times, some approach like simply add the [id] at the line end of LocalizedMessage don't work.

I find it a quite complicated problem while trying to read and understand all the codes about messages. It took me a long time and I was thinking that completing the first version of the regression tool is more important, so I just work on this bite by bite trying more proper approaches.

I am now paying more attention on this. But still changes are producing new failure and not really easy.

Contributor

Luolc commented Jul 31, 2017

@rnveach @romani
There are so many references of the getMessage method which used for printing the message. I have tried some ways to add the id, but all of the ways would affect UTs a lot. I spend a lot of time fixing UTs and keep rebasing the laster commit to avoid no new failure. Also I have reverted my work some times, some approach like simply add the [id] at the line end of LocalizedMessage don't work.

I find it a quite complicated problem while trying to read and understand all the codes about messages. It took me a long time and I was thinking that completing the first version of the regression tool is more important, so I just work on this bite by bite trying more proper approaches.

I am now paying more attention on this. But still changes are producing new failure and not really easy.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 31, 2017

Member

many references of the getMessage method which used for printing the message
I have tried some ways to add the id, but all of the ways would affect UTs a lot.

99% of our UTs (I would say) don't reference an ID for the module, so they shouldn't be affected by this change.
Someone did start some code before you at https://github.com/checkstyle/checkstyle/pull/4626/files .
Can we start with the change they did in LocalizedMessage as I believe this is the cause of the bug I mentioned at #4607 (comment) .

The other 2 files changed are on the right path, but need to be modified to only show the ID instead of the module name and ID.

changes are producing new failure and not really easy.

If you start a PR with this, I can guide you more on the failing tests as even I don't know how much will be affected by coming changes.

Member

rnveach commented Jul 31, 2017

many references of the getMessage method which used for printing the message
I have tried some ways to add the id, but all of the ways would affect UTs a lot.

99% of our UTs (I would say) don't reference an ID for the module, so they shouldn't be affected by this change.
Someone did start some code before you at https://github.com/checkstyle/checkstyle/pull/4626/files .
Can we start with the change they did in LocalizedMessage as I believe this is the cause of the bug I mentioned at #4607 (comment) .

The other 2 files changed are on the right path, but need to be modified to only show the ID instead of the module name and ID.

changes are producing new failure and not really easy.

If you start a PR with this, I can guide you more on the failing tests as even I don't know how much will be affected by coming changes.

Luolc added a commit to Luolc/checkstyle that referenced this issue Aug 4, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Aug 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Aug 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Aug 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Aug 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Aug 7, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Aug 7, 2017

romani added a commit that referenced this issue Aug 10, 2017

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

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 10, 2017

Member

fix is merged

Member

romani commented Aug 10, 2017

fix is merged

@romani romani closed this Aug 10, 2017

ArneLimburg pushed a commit to ArneLimburg/checkstyle that referenced this issue Aug 23, 2017

ArneLimburg pushed a commit to ArneLimburg/checkstyle that referenced this issue Aug 23, 2017

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