Skip to content
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

Issue: #7696 update doc for RegexpOnFilename #7953

Merged
merged 1 commit into from Apr 22, 2020

Conversation

hembhagat99
Copy link
Contributor

@hembhagat99 hembhagat99 commented Mar 24, 2020

Issue #7696: update doc for RegexpOnFilename

Examples Added:

Changes4

Changes2

Changes3

CLI Results:

For default configuration
config.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC 
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
	<module name="RegexpOnFilename"/>
</module>

Results

F:\College\Checkstyle Jar>java -jar checkstyle-8.30-all.jar -c config.xml src/xdocs/"config regexp.xml"
Starting audit...
[ERROR] F:\College\Checkstyle Jar\src\xdocs\config regexp.xml:1: File match folder pattern '' and file pattern '\s'. [RegexpOnFilename]
Audit done.
Checkstyle ends with 1 errors.

For custom configurations
config.xml

<?xml version="1.0"?>                                           
<!DOCTYPE module PUBLIC                                         
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN" 
          "https://checkstyle.org/dtds/configuration_1_3.dtd">  
        
<module name="Checker">
<module name="RegexpOnFilename">
<property name="folderPattern" value="[\\/]src"/>
<property name="fileNamePattern" value="\.(java|xml)$"/>
<property name="match" value="false"/>
</module>
</module>

Results

F:\College\Checkstyle Jar>java -jar checkstyle-8.30-all.jar -c config.xml src\main_properties.properties
Starting audit...
[ERROR] F:\College\Checkstyle Jar\src\main_properties.properties:1: File not match folder pattern '[\\/]src' and file pattern '\.(java|xml)$'. [RegexpOnFilename]
Audit done.
Checkstyle ends with 1 errors.

F:\College\Checkstyle Jar>java -jar checkstyle-8.30-all.jar -c config.xml src\NewClass.java
Starting audit...
Audit done.

New Configuration
To configure the check to forbid 'md' files except 'README.md file' in folders, with custom message:

config.xml

<?xml version="1.0"?>                                           
<!DOCTYPE module PUBLIC                                         
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN" 
          "https://checkstyle.org/dtds/configuration_1_3.dtd">  
        
<module name="Checker">
	<module name="RegexpOnFilename">
		<property name="fileNamePattern" value="README"/>
		<property name="fileExtensions" value="md"/>
		<property name="match" value="false"/>
		<message key="regexp.filename.mismatch"
		value="No '*.md' files other then 'README.md'"
		/>
	</module>
</module>

Results

F:\College\Checkstyle Jar\src>java -jar checkstyle-8.30-all.jar -c config.xml README.md
Starting audit...
Audit done.

F:\College\Checkstyle Jar\src>java -jar checkstyle-8.30-all.jar -c config.xml hello.md
Starting audit...
[ERROR] F:\College\Checkstyle Jar\src\hello.md:1: No *.md files other then README.md [RegexpOnFilename]
Audit done.
Checkstyle ends with 1 errors.

F:\College\Checkstyle Jar\src>java -jar checkstyle-8.30-all.jar -c config.xml NewClass.class
Starting audit...
Audit done.

@timurt
Copy link
Contributor

timurt commented Mar 24, 2020

@DeveloperHB please follow instructions from here https://github.com/checkstyle/checkstyle/wiki/Good-practice-on-how-to-update-examples

Update PR description with images and cli outputs

@hembhagat99
Copy link
Contributor Author

@timurt @romani I have updated the PR description.

I would also like to draw your attention to this.

While I was trying different configurations, I found errors in the configurations which contains attribute folderPattern. It was not working as expected. So, I had a look at the Java code in RegexpOnFilename.java file and I found this.

Consider this configuration,

<module name="RegexpOnFilename">
  <property name="folderPattern" value="[\\/]src[\\/]"/>
  <property name="fileNamePattern" value="\.(java|xml)$"/>
  <property name="match" value="false"/>
</module>

Consider this file,

F:\examples\src\main_properties.properties

The code which gets the folder pattern of the file is,

File file = new File("main_properties.properties");
file.getCanonicalFile().getParent();

Consider java file to be in src folder.

The return value of the statement file.getCanonicalFile().getParent(); in this case is F:\examples\src. But the regular expression in the configuration is [\\/]src[\\/]. So, it tries to find a match for \src\ which leads to unexpected behavior.

So, I recommend to change
[\\/]src[\\/] ---> [\\/]src in the configuration and perform corresponding changes in every other similar cases.

I am working on windows. So, this can also be problem of operating system. In that case we can perform following change,

[\\/]src[\\/] ---> [\\/]src[\\/]*

It will try to match 0 or more occurrences of the last slash(/ or \).

@hembhagat99
Copy link
Contributor Author

@timurt @romani What I have to do for unsuccessful checks.

@rnveach
Copy link
Member

rnveach commented Mar 25, 2020

Consider this file,
F:\examples\src\main_properties.properties

No files should be located in the src folder. We follow the maven pattern and all resources should be located in \src\main\resources or \src\test\resources.

If you are referring to the documentation and it gives a src folder example, then please ensure documentation is correct and it will give a violation is described. If documentation is wrong or doesn't work on certain OSes, then it should be updated or the discrepancy explained.

What I have to do for unsuccessful checks.

https://github.com/checkstyle/checkstyle/wiki/PR-rules
CI must be green before we start reviewing it. If you have issues or don't understand CI failure then let us know. We recommend running mvn verify on your local branch before pushing to the server.

@hembhagat99
Copy link
Contributor Author

If you are referring to the documentation and it gives a src folder example, then please ensure documentation is correct and it will give a violation is described. If documentation is wrong or doesn't work on certain OSes, then it should be updated or the discrepancy explained.

Currently there is no documentation available which provides examples for RegexpOnFilename. I have the edited the documentation to provide some examples. When I was trying different configurations, I found this problem.

As of my point of view, the configurations mentioned in the documentation contain some error. I also recommend to check this issue with other Windows users. We should also open this as an issue if more users are having same problem.

CI must be green before we start reviewing it. If you have issues or don't understand CI failure then let us know. We recommend running mvn verify on your local branch before pushing to the server.

I already run mvn clean verify command on my local branch and got Build Success.

From the error messages of the check which failed,
The error occurs in grammar generation

Please visit below link to see full log of the check.
https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/31685182/job/14ywvg7q0ddc62xa

@hembhagat99
Copy link
Contributor Author

What is the status of this PR?

@hembhagat99
Copy link
Contributor Author

hembhagat99 commented Mar 30, 2020

@romani @timurt @rnveach

Sorry, I misunderstood the error message in CI check. As the contents had red background, I understood that as an error.

The actual error is in commit message. The CI check gets following commit message,

Issue #7696: update doc for RegexpOnFilename\n\nIssue #7696: update doc for RegexpOnFilename\n

I only have one commit in the Pull Request.

What should be the reason behind this?
How can I solve this?

@wltan
Copy link
Contributor

wltan commented Mar 31, 2020

Your commit has a description attached. It should be a single line only (just the message).

image

image
^ rule 2 broken

@hembhagat99
Copy link
Contributor Author

Thank you @wltan . Can you please suggest me a way to remove this?

@wltan
Copy link
Contributor

wltan commented Mar 31, 2020

https://help.github.com/en/github/committing-changes-to-your-project/changing-a-commit-message

You could try the git commit --amend command from the above link or a git rebase -i HEAD~1 and force push.

@hembhagat99 hembhagat99 force-pushed the 7696-regexponfilename-doc branch 2 times, most recently from 5f5a7d9 to e90d95a Compare March 31, 2020 16:15
@hembhagat99
Copy link
Contributor Author

Thank you @wltan . It worked. Now I am getting this error wercker/build — Wercker pipeline failed . Also I am unable to see its details. The site says 401.

Here is the link
https://app.wercker.com/checkstyle/checkstyle/runs/build/5e836c967b63df001af72b83

@hembhagat99
Copy link
Contributor Author

@romani I am getting wrecker CI check error in this PR. I have already rebase it to latest master on 31st March. Please help to solve this.

@hembhagat99 hembhagat99 force-pushed the 7696-regexponfilename-doc branch 2 times, most recently from cbbd3dd to 0a7a849 Compare April 4, 2020 14:27
@hembhagat99 hembhagat99 force-pushed the 7696-regexponfilename-doc branch 2 times, most recently from 1e42e2f to 8b25638 Compare April 17, 2020 12:31
@romani romani self-assigned this Apr 19, 2020
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

items to improve:

@hembhagat99 hembhagat99 force-pushed the 7696-regexponfilename-doc branch 2 times, most recently from 2dde891 to 5b52b56 Compare April 20, 2020 04:10
@romani
Copy link
Member

romani commented Apr 20, 2020

Please do not forget to regenerate PR description image on how web page is rendering. Please reply "done" to each point.

@hembhagat99
Copy link
Contributor Author

@romani After the changes, the mvn clean verify is giving error. The error is of XdocsJavaDocsTest.
As far as I know this error occurs when content of JavaDoc and config_regexp.xml doesn't match. I checked it twice but I don't find anything missing in the JavaDoc. Maybe it will be very small to detect. Is there any tool using which we can catch the mistake? Please help.

To see the error log, visit this link https://travis-ci.org/github/checkstyle/checkstyle/jobs/677091722

@hembhagat99
Copy link
Contributor Author

Once I solve this error I will update the PR description with new images.

@romani
Copy link
Member

romani commented Apr 21, 2020

it is easy to resolve yourself, all is printed in logs output.
I copied them and compared - https://www.diffchecker.com/M7TJtkKE .

@hembhagat99
Copy link
Contributor Author

Thank you @romani . It was an extra ; that was creating trouble. I have pushed the changes and will update the description.

I would also like you to see this comment #7953 (comment). I also recommend the changes described in the comment.

@romani
Copy link
Member

romani commented Apr 21, 2020

I also recommend the changes described in the comment.

You can add it to this PR, or better to create new issue and show problem by CLI.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok to merge when CI pass.

@DeveloperHB , please move update to trailing * in folder regexp to separate issue/PR.
It is better to merge this PR sooner.

@hembhagat99
Copy link
Contributor Author

@romani I suggest to create a new issue for adding trailing * and marking it as a good first issue. It will help new contributors to develop better understanding about the checkstyle project.

@hembhagat99
Copy link
Contributor Author

hembhagat99 commented Apr 21, 2020

@romani The Travis CI is saying that you are providing PR for issue that is not approved. Please help.
This is the link to the log https://travis-ci.org/github/checkstyle/checkstyle/jobs/677717508

This is solved.

@romani
Copy link
Member

romani commented Apr 21, 2020

I suggest to create a new issue for adding trailing * and marking it as a good first issue.

Please create issue, as you already have all details.

@romani romani merged commit 016017c into checkstyle:master Apr 22, 2020
@hembhagat99
Copy link
Contributor Author

@romani When will be the changes reflected in the website https://checkstyle.org/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants