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

Reorganize token methods of all Checks #4581

Closed
rnveach opened this Issue Jul 2, 2017 · 14 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Jul 2, 2017

Broken off from #605 ,

Acceptable >= Default >= Required

As explained from gitter:

getAcceptableTokens should call getRequiredTokens
getRequiredTokens should call for return new int[] {TokenTypes.BLOCK_COMMENT_BEGIN };
all other call getRequiredTokens

This change should be done to both Checkstyle and Sevntu on all checks.

@rnveach rnveach added the approved label Jul 2, 2017

@romani romani added the easy label Jul 3, 2017

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jul 4, 2017

Contributor

@rnveach

getAcceptableTokens should call getRequiredTokens
getRequiredTokens should call for return new int[] {TokenTypes.BLOCK_COMMENT_BEGIN };
all other call getRequiredTokens

Could you please elaborate a little on this?

Contributor

subkrish commented Jul 4, 2017

@rnveach

getAcceptableTokens should call getRequiredTokens
getRequiredTokens should call for return new int[] {TokenTypes.BLOCK_COMMENT_BEGIN };
all other call getRequiredTokens

Could you please elaborate a little on this?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 11, 2017

Member

@subkrish ,

look at commit dfa3cee
for changes like:

+    @Override
+    public final int[] getAcceptableTokens() {
+        return getDefaultTokens();
+    }
+
+    @Override
+    public final int[] getRequiredTokens() {
+        return getDefaultTokens();
+    }

code should be like:

    @Override
    public final int[] getDefaultTokens() {
        return getRequiredTokens();
    }

    @Override
    public final int[] getAcceptableTokens() {
        return getRequiredTokens();
    }

    @Override
    public final int[] getRequiredTokens() {
        return new int[] {.......................};
    }
Member

romani commented Jul 11, 2017

@subkrish ,

look at commit dfa3cee
for changes like:

+    @Override
+    public final int[] getAcceptableTokens() {
+        return getDefaultTokens();
+    }
+
+    @Override
+    public final int[] getRequiredTokens() {
+        return getDefaultTokens();
+    }

code should be like:

    @Override
    public final int[] getDefaultTokens() {
        return getRequiredTokens();
    }

    @Override
    public final int[] getAcceptableTokens() {
        return getRequiredTokens();
    }

    @Override
    public final int[] getRequiredTokens() {
        return new int[] {.......................};
    }

subkrish added a commit to subkrish/checkstyle that referenced this issue Oct 2, 2017

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Oct 2, 2017

Contributor

@romani @rnveach I am on it.

Contributor

subkrish commented Oct 2, 2017

@romani @rnveach I am on it.

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Oct 10, 2017

Contributor

@rnveach @romani

    @Override
    public int[] getDefaultTokens() {
        return new int[] {
            TokenTypes.DOT,
            TokenTypes.COMMA,
        };
    }

    @Override
    public int[] getAcceptableTokens() {
        return new int[] {
            TokenTypes.DOT,
            TokenTypes.COMMA,
            TokenTypes.SEMI,
            TokenTypes.ELLIPSIS,
            TokenTypes.AT,
            TokenTypes.LPAREN,
            TokenTypes.RPAREN,
            TokenTypes.ARRAY_DECLARATOR,
            TokenTypes.RBRACK,
            TokenTypes.METHOD_REF,
        };
    }

    @Override
    public int[] getRequiredTokens() {
        return CommonUtils.EMPTY_INT_ARRAY;
    }

How do you suggest altering this kind of set up?

Contributor

subkrish commented Oct 10, 2017

@rnveach @romani

    @Override
    public int[] getDefaultTokens() {
        return new int[] {
            TokenTypes.DOT,
            TokenTypes.COMMA,
        };
    }

    @Override
    public int[] getAcceptableTokens() {
        return new int[] {
            TokenTypes.DOT,
            TokenTypes.COMMA,
            TokenTypes.SEMI,
            TokenTypes.ELLIPSIS,
            TokenTypes.AT,
            TokenTypes.LPAREN,
            TokenTypes.RPAREN,
            TokenTypes.ARRAY_DECLARATOR,
            TokenTypes.RBRACK,
            TokenTypes.METHOD_REF,
        };
    }

    @Override
    public int[] getRequiredTokens() {
        return CommonUtils.EMPTY_INT_ARRAY;
    }

How do you suggest altering this kind of set up?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 12, 2017

Member

@subkrish , please skip such Checks. If Required tokens are empty, no valuable refactoring could happen. We should not change logic of Check.

Member

romani commented Oct 12, 2017

@subkrish , please skip such Checks. If Required tokens are empty, no valuable refactoring could happen. We should not change logic of Check.

subkrish added a commit to subkrish/checkstyle that referenced this issue Oct 17, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Oct 29, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Oct 30, 2017

romani added a commit that referenced this issue Nov 3, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Nov 13, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Nov 13, 2017

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

subkrish added a commit to subkrish/checkstyle that referenced this issue Nov 14, 2017

subkrish added a commit to subkrish/checkstyle that referenced this issue Nov 14, 2017

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

subkrish added a commit to subkrish/checkstyle that referenced this issue Nov 14, 2017

rnveach added a commit that referenced this issue Nov 15, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 26, 2017

Member

@subkrish , do you still have smth to contribute in this issue ?

Member

romani commented Nov 26, 2017

@subkrish , do you still have smth to contribute in this issue ?

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Nov 28, 2017

Contributor

@romani As far as I am concerned the work on this issue is done but there are still some files that have to be changed. But upon changing it is causing it to lose mutation coverage. So any file that causes this has not been changed. I do not know how to go about such issues.

Contributor

subkrish commented Nov 28, 2017

@romani As far as I am concerned the work on this issue is done but there are still some files that have to be changed. But upon changing it is causing it to lose mutation coverage. So any file that causes this has not been changed. I do not know how to go about such issues.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 28, 2017

Member

upon changing it is causing it to lose mutation coverage
I do not know how to go about such issues.

@subkrish It depends on the missing mutation, but since this is all revolving around the token methods, all you probably need to do is add a test to verify their values.
Example: https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationOnSameLineCheckTest.java#L47-L69

If you build the mutation report when you have problems, we can guide you when your having problems.

Member

rnveach commented Nov 28, 2017

upon changing it is causing it to lose mutation coverage
I do not know how to go about such issues.

@subkrish It depends on the missing mutation, but since this is all revolving around the token methods, all you probably need to do is add a test to verify their values.
Example: https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationOnSameLineCheckTest.java#L47-L69

If you build the mutation report when you have problems, we can guide you when your having problems.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 28, 2017

Member

Please provide list of Checks that still need to updated.
Please share a pitest report for them , and we will guide you

Member

romani commented Nov 28, 2017

Please provide list of Checks that still need to updated.
Please share a pitest report for them , and we will guide you

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jan 4, 2018

Contributor

@romani @rnveach

Sorry I'm coming back to this after a long time. But I finally got back to working on this issue. I generated the report for the naming checks.

Report: https://subkrish.github.io/

So based on this, how do I move forward?

Contributor

subkrish commented Jan 4, 2018

@romani @rnveach

Sorry I'm coming back to this after a long time. But I finally got back to working on this issue. I generated the report for the naming checks.

Report: https://subkrish.github.io/

So based on this, how do I move forward?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 6, 2018

Member

@subkrish Please confirm all methods are done in all checks.

Member

rnveach commented Jan 6, 2018

@subkrish Please confirm all methods are done in all checks.

@rnveach rnveach added this to the 8.8 milestone Jan 6, 2018

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Jan 6, 2018

Contributor

@rnveach @romani

Yes, I checked to make sure all methods have been changed in all checks keeping in mind the ones @romani asked me not to change (#4581 (comment)) in these type of checks #4581 (comment).

This issue can be closed.

Contributor

subkrish commented Jan 6, 2018

@rnveach @romani

Yes, I checked to make sure all methods have been changed in all checks keeping in mind the ones @romani asked me not to change (#4581 (comment)) in these type of checks #4581 (comment).

This issue can be closed.

@rnveach rnveach closed this Jan 6, 2018

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 6, 2018

Member

Thanks.

Member

rnveach commented Jan 6, 2018

Thanks.

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