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

RegexpHeader not detecting '\n\n' by regex properly #4735

Closed
vivekrao1985 opened this Issue Jul 16, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@vivekrao1985

vivekrao1985 commented Jul 16, 2017

I'm trying to add a checkstyle config for how code files should begin in the codebase. The format I want can be described by a pattern in java - ^package .*\n\n.*. However, this pattern doesn't seem to work when I use it with RegexpHeader.

Here are steps to reproduce -

$ cat ShouldFail.java
package my.checkstyle.frustrations;
/**
 * Hello world!
 *
 */
public class ShouldFail { }

$ cat check-style.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.2//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_2.dtd">
<module name="Checker">
    <module name="RegexpHeader">
        <property name="header" value="^package .*\n\n.*"/>
        <property name="fileExtensions" value="java"/>
    </module>
</module>
$ java -jar checkstyle-8.0-all.jar -c check-style.xml ShouldFail.java
Starting audit...
Audit done.

Expected -

Starting audit...
[ERROR] ShouldFail.java:1: Line does not match expected header line of '^package .*\n\n.*'. [RegexpHeader]
Audit done.

Based on my regular expression, I expect checkstyle to pass for this -

package my.checkstyle.frustrations;

/**
 * @author Vivek Rao
 */
public class ShouldPass { }

It should also pass for this -

package my.checkstyle.frustrations;

import java.lang.instrument.ClassDefinition;

/**
 * @author Vivek Rao
 */
public class ShouldAlsoPass { } 

Here is a link to the source code.

@vivekrao1985

This comment has been minimized.

Show comment
Hide comment
@vivekrao1985

vivekrao1985 Jul 17, 2017

I think the problem is in AbstractHeaderCheck. If the input header regex has two or more consecutive newlines, then the default behavior is to accept everything, when it should really be checking to make sure the line is empty. I managed to bypass this issue by defining my regex as ^package\n^$. So basically ^package enforces that the first line of a code file must begin with package and \n^$ enforces that 2nd line is empty.

I think to fix this the right way we should replace blank lines with ^$ in AbstractHeaderCheck. Thoughts?

vivekrao1985 commented Jul 17, 2017

I think the problem is in AbstractHeaderCheck. If the input header regex has two or more consecutive newlines, then the default behavior is to accept everything, when it should really be checking to make sure the line is empty. I managed to bypass this issue by defining my regex as ^package\n^$. So basically ^package enforces that the first line of a code file must begin with package and \n^$ enforces that 2nd line is empty.

I think to fix this the right way we should replace blank lines with ^$ in AbstractHeaderCheck. Thoughts?

@romani romani changed the title from Checkstyle RegexpHeader not detecting regex properly to RegexpHeader not detecting '\n\n' by regex properly Jul 19, 2017

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

@checkstyle checkstyle deleted a comment from vivekrao1985 Jul 19, 2017

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

@checkstyle checkstyle deleted a comment from vivekrao1985 Jul 19, 2017

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

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 19, 2017

Member

I think to fix this the right way we should replace blank lines with ^$ in AbstractHeaderCheck. Thoughts?

Good idea, please be welcome with PullRequest if you have time to fix this.

Member

romani commented Jul 19, 2017

I think to fix this the right way we should replace blank lines with ^$ in AbstractHeaderCheck. Thoughts?

Good idea, please be welcome with PullRequest if you have time to fix this.

@romani romani added the approved label Jul 19, 2017

@vivekrao1985

This comment has been minimized.

Show comment
Hide comment
@vivekrao1985

vivekrao1985 Jul 19, 2017

@romani sure, I will open a PullRequest for this.

vivekrao1985 commented Jul 19, 2017

@romani sure, I will open a PullRequest for this.

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Jul 19, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Jul 19, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Jul 19, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Aug 2, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Aug 2, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Aug 2, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Aug 2, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Aug 2, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Aug 3, 2017

vivekrao1985 pushed a commit to vivekrao1985/checkstyle that referenced this issue Aug 3, 2017

rnveach added a commit that referenced this issue Aug 3, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 3, 2017

Member

Fix was merged

Member

rnveach commented Aug 3, 2017

Fix was merged

@rnveach rnveach closed this Aug 3, 2017

@rnveach rnveach added this to the 8.2 milestone Aug 3, 2017

@romani romani added the bug label Aug 4, 2017

soon added a commit to soon/checkstyle that referenced this issue Aug 29, 2017

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