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

Line numbers should start at 1 #6000

Closed
rnveach opened this issue Jul 4, 2018 · 5 comments
Closed

Line numbers should start at 1 #6000

rnveach opened this issue Jul 4, 2018 · 5 comments
Milestone

Comments

@rnveach
Copy link
Member

@rnveach rnveach commented Jul 4, 2018

Similar to #4997 ,

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocPackageCheck.java#L89

CheckstyleBad.java:0: Missing package-info.java file

AST checks start at line 1. We should remove line 0 violations and make them start at 1.
If the violation, like the above, is on a missing file or the file as a whole, we should remove the line number completely and just print the file name.

Example: CheckstyleBad.java: Missing package-info.java file

@romani

This comment has been minimized.

Copy link
Member

@romani romani commented Jul 4, 2018

it is better to print violation on some AST node, package node is good candidate.

@romani romani added the approved label Jul 4, 2018
@rnveach

This comment has been minimized.

Copy link
Member Author

@rnveach rnveach commented Jul 4, 2018

@romani this is a file set check, there is no ast. Also even though violation is on file X, it's message is saying file Y is missing.

@romani

This comment has been minimized.

Copy link
Member

@romani romani commented Jul 4, 2018

Ok, let's just have it be reported to first line - 1.

@li-boxuan

This comment has been minimized.

Copy link
Contributor

@li-boxuan li-boxuan commented Aug 29, 2018

Hi, I'd like to solve this issue. Before creating a pull request, can we confirm one thing: do we want to report 1 or just print the file name?

UPDATE:
I use 1 for now because it's much easier. I can change it to just print the file name if it is required.

li-boxuan added a commit to li-boxuan/checkstyle that referenced this issue Aug 30, 2018
li-boxuan added a commit to li-boxuan/checkstyle that referenced this issue Aug 30, 2018
li-boxuan added a commit to li-boxuan/checkstyle that referenced this issue Aug 31, 2018
li-boxuan added a commit to li-boxuan/checkstyle that referenced this issue Aug 31, 2018
li-boxuan added a commit to li-boxuan/checkstyle that referenced this issue Aug 31, 2018
@rnveach rnveach closed this in #6095 Sep 5, 2018
rnveach added a commit that referenced this issue Sep 5, 2018
@rnveach

This comment has been minimized.

Copy link
Member Author

@rnveach rnveach commented Sep 5, 2018

Fix was merged

@rnveach rnveach added this to the 8.13 milestone Sep 5, 2018
jack870131 added a commit to jack870131/checkstyle that referenced this issue Dec 3, 2018
jack870131 added a commit to jack870131/checkstyle that referenced this issue Dec 3, 2018
tsjensen added a commit to checkstyle-addons/checkstyle-addons that referenced this issue Mar 22, 2019
when the entire file is referred to by the flagged issue. This becomes
necessary because of checkstyle/checkstyle#6000, where Checkstyle 8.13
silently introduced this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.