#334 New check SingleSpace

133 views
Skip to first unread message

Robert Whitebit

unread,
Jul 24, 2015, 12:21:27 PM7/24/15
to Sevntu Checkstyle
Hi!

I just created a first approach for the single space check issue. Before continuing, I want to get some feedback if this is the right way to go.


Coding guidelines and other stuff will be adapted after some feedback. I just used the Netbeans code formatter atm.

Thanks :)

Robert Whitebit

unread,
Jul 25, 2015, 2:36:32 PM7/25/15
to Sevntu Checkstyle, robert....@gmail.com
The check also works for comments now.

In the sevntu project, most single whitespace errors are in the license :D
2x JavadocTokenTypes.java#L17

Also the gnu license v3 (http://www.gnu.org/licenses/gpl-3.0.en.html) has two single spaces on the last line. -.-
[...program.  If not,...]

I excluded the comment indentation for the check on purpose because these following examples should not report an error:
Single-line comments: (commented code)

Block comments:

The indentation in a single-line comment are leading whitespaces, whereas in a block commit it's every leading whitespace and if a * character is used, every whitespace after it.

However, we can add properties to define if the indentation in comments should be ignored. (There is a commit in the branch where indentation isn't ignored already)


Horizontal alignment(http://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-alignment) is something which changes the check from easy to hard imo, I don't see any easy way to achieve that and keep it also reliable yet.

Robert Whitebit

unread,
Jul 26, 2015, 8:04:54 AM7/26/15
to Sevntu Checkstyle, robert....@gmail.com
I created a PR for inspection and feedback at https://github.com/robertwhitebit/sevntu.checkstyle/pull/1/files Keep in mind, that I used the netbeans formatter and have to remove a few debug outputs and comments.
I'll also test other projects besides the sevntu project.

Settings and which errors should be reported or ignored should be discussed as well on github, I guess.

Roman Ivanov

unread,
Aug 2, 2015, 10:30:18 PM8/2/15
to sevntu-c...@googlegroups.com, robert....@gmail.com
Hi Robert,

sorry for being silent, I will reply you 100% in first days of September, I am on vacation.
you are on my todo list.

thanks,
Roman Ivanov

--
You received this message because you are subscribed to the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sevntu-checkst...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Robert Whitebit

unread,
Aug 3, 2015, 7:05:58 AM8/3/15
to Sevntu Checkstyle, robert....@gmail.com
No problem. Thanks already for your reply!
Enjoy your holiday :)

Tom Verhoeff

unread,
Sep 7, 2015, 6:08:32 AM9/7/15
to Sevntu Checkstyle
Hi all,

romani asked me to join this discussion.

I suggested in issue #2090 to add a boolean option allowMultipleWhitespaces to WhiteSpaceAfter and WhiteSpaceAround, in analogy to boolean option allowMultipleEmptyLines in check EmptyLineSeparator.

By relating it as an option to specific checks, it allows better fine tuning.

On the other hand, if you really want to enforce single spaces everywhere (except in indentation and string literals), then you would have to set an option in multiple places. Currently, the following checks involve whitespace in specific places:

  • GenericWhitespace
  • EmptyForInitializerPad
  • EmptyForIteratorPad
  • MethodParamPad
  • ParenPad
  • TypecastParenPad
  • WhitespaceAfter
  • WhitespaceAround
The pad checks have an option for pad policy (space, or nospace); it could be made to differentiate between singlespace and one-or-more-space.

The Whitespace* checks would also need an option.

To me, it would make sense to make the single space option the default.

Would that not also solve issue #334?

Robert Whitebit

unread,
Sep 8, 2015, 10:38:17 AM9/8/15
to Sevntu Checkstyle
Hi! :)

The check for a single space is really easy. Link. With some work, it should be able to be integrated in the listed checks smoothly.
However, before doing so, there are a few things to consider:
  • Currently, the implemented check checks comments as well, as described in the issue. Instead of just checking spaces, there should be a complete new check which checks the style of the comment (including Javadoc). No multiple spaces in text, correct aligning, etc.
  • All listed whitespace checks are checking whitespaces only. Whitespace != space. Since the policy names are already space and nospace, I see no problem here.
  • How to combine aligned code like here and space checks? Maybe more important in the future.
  • The current implementation doesn't care if there has to be a space or not. It just checks, if there are whitespaces between tokens they have to be a single space. I just guess that all listed checks covers all (important) tokens.
I like your solution as well. However, I'll wait for feedback of one of the project maintainers before starting to implement it this way.

Roman Ivanov

unread,
Sep 29, 2015, 9:13:01 PM9/29/15
to sevntu-c...@googlegroups.com
Hi Robert,

> Horizontal alignment(http://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-alignment) is something which changes the check from easy to hard imo, I don't see any easy way to achieve that and keep it also reliable yet.
> I excluded the comment indentation for the check on purpose because these following examples should not report an error:

Lest exclude comments analysis completely, comments are used for pseudo-graphic and other stuff that user need.
We will run into saint war with user of what is correct and what is not.
I would recommend to release first version for code, if Check works fine try to support comments validation by special option.


1)
> 
public static File currentFile;
public static boolean reportErrors = true;

please make them as non-static.

2)
> 
private void checkSingleSpaceBeforeToken(DetailAST ast) {
if (ast.getColumnNo() < 2) {
return;
}

Please move this IF to upper level; make method return boolean and log event in VisitEachToken; make a method as static(it could be pure logic method, without side effect)


3) 
> sevntu-checks/src/test/resources/com/github/sevntu/checkstyle/checks/whitespace/SingleNoSpaceErrors.java

please name all test input files as "Input......" , we have that rule in main project.

4) 
lets proceed with sharing testing reports. we use checkstyle-tester project , you can see example at https://groups.google.com/forum/#!topic/sevntu-checkstyle/cQG-PA0WgvA

thanks,
Roman Ivanov
​

Roman Ivanov

unread,
Sep 29, 2015, 9:30:05 PM9/29/15
to sevntu-c...@googlegroups.com
Hi Robert,

see my reply to Tom Verhoeff

We need to detach your ideas, each of you make sense.

thanks,
Roman Ivanov
​

Robert Whitebit

unread,
Oct 9, 2015, 3:14:10 PM10/9/15
to Sevntu Checkstyle
Comments analysis has been removed completely. Also the ability to test projects because I wasn't aware of the checkstyle-tester project (solves @1). This shrinks the code drastically.

@2: I changed everything except "move this IF to upper level". The IF is a guard clause to prevent an IndexOutOfBoundsException.
@3: Done!

@4: On it. I get a StringIndexOutOfBoundsException: String index out of range: 131 at InputNewlineCrAtEndOfFile.java in the Checkstyle project.
Text: */
Type: 182 (BLOCK_COMMENT_END)
LineNo: 7, ColumnNo: 131, Line: 0

There seem to be a problem with CR line-endings. If you open the file, you can see that the BLOCK_COMMENT_END is on line 11, not 7 nor 0. Also the columnNo is completely wrong.
So there is a bug somewhere in the core of the project. :(

Besides that, I uploaded the results of the guava and sevntu.checkstyle project. Hopefully the links will work soon.

Roman Ivanov

unread,
Oct 10, 2015, 7:10:19 PM10/10/15
to sevntu-c...@googlegroups.com
Hi Robert,

1)
It will also return true if the token is the first token on the line or there is no
* whitespace between the token and the previous token.
if (ast.getColumnNo() < 2) {
return true;
}

Just make from this condition separate function, and name it properly, and you will not need detailed javadoc



2)
> isSeparatedBySingleSpaceWithPreviousToken

there should be single exit point. Almost all methods should have single exit points.

3)
> String line;

are you sure that you get performance boost by placing declaration out of WHILE ?


4)
> There seem to be a problem with CR line-endings.

Formatting did not make me to understand a problem - please file issue to github with minimalistic input file to let reproduce a problem. And print a tree structure that you see (https://github.com/checkstyle/contribution/tree/master/javadoc-tree-serializer).

5)
> Besides that, I uploaded the results of the guava and sevntu.checkstyle project. Hopefully the links will work soon.

both links give me 404. please recheck.

thanks,
Roman Ivanov
​

Robert Whitebit

unread,
Oct 12, 2015, 9:10:01 AM10/12/15
to Sevntu Checkstyle
1,2,3) Done! I extracted some code to methods. Can you please provide feedback for the method again. Especially about the comment and how to handle it.


5) Done: guava and sevntu.checkstyle
guava has lots of violations because they use multiple spaces before the opening comment tag.

Roman Ivanov

unread,
Oct 13, 2015, 10:12:37 AM10/13/15
to sevntu-c...@googlegroups.com
Hi Robert,

Code looks good. thanks for reporting an issue.

Please always put in your reply link to commit, that helps me a lot.

1) please make coverage to 100%, we are about to finish acceptance of your Check.
please 

2) Introduction of new Check in sevntu is a bit more complicated, Please update your commit to have changes in all files as at


3) looks like a false positive

4)
Please send us PR with fixed violations and lets make sure that after all fixes, report is empty and code is compilable. Share empty report on github.io as prove.


5)
Check will not be welcomed community due to its limitation to skip such code:

6)
There are a lot of cases of double space to separate trailing comment.
Example:
Looks like they do this for a reason that we do not know. I propose to do PR for some cases to guava project a see what they answer. 


thanks,
Roman Ivanov
​

Robert Whitebit

unread,
Oct 13, 2015, 2:34:49 PM10/13/15
to Sevntu Checkstyle
Thanks for your feedback. :)


On Tuesday, October 13, 2015 at 4:12:37 PM UTC+2, RomanIvanov wrote:
1) please make coverage to 100%, we are about to finish acceptance of your Check.
please 

Currently the branchRate is 96.8%, the lineRate is 100%.
Is there a way to get some kind of visual information? I wasn't able to generate a index.html file or somehow visualize the cobertura.ser file :(
Nice catch! A quick debugging session revealed that the comment is preceded by a tab character (which looks like a single space in these cases). So these cases look like false positive but aren't.
 
6)
There are a lot of cases of double space to separate trailing comment.
Example:
Looks like they do this for a reason that we do not know. I propose to do PR for some cases to guava project a see what they answer.

Kevin Bourrillion: "Some people apparently like to do it for some reason, and our style guide allows it."

Roman Ivanov

unread,
Oct 13, 2015, 3:48:12 PM10/13/15
to sevntu-c...@googlegroups.com
> Is there a way to get some kind of visual information?

mvn cobertura:cobertura will generate HTML report with all details line by line 

> preceded by a tab character

one tab is one character, why not false positive ?


Any proposal how to add support for this in check ? to let skip such trailing comments.

​

Robert Whitebit

unread,
Oct 13, 2015, 6:07:50 PM10/13/15
to Sevntu Checkstyle
mvn cobertura:cobertura will generate HTML report with all details line by line 

Ah, thank you very much. 100% now :D

> preceded by a tab character
one tab is one character, why not false positive ?

Tokens should be separated by a single space character. A tab between tokens may look like the following example and will be replaced by a formatter anyway.
int a    = b;

I also quickly tested the default behavior of the Android Studio, Eclipse and Netbeans formatter. Tabs between tokens will always be replaced by a single space. Except for comments :D The outputs of the formatters are really complex. Sometimes tabs get replaced by spaces or a single space. Sometimes don't. Sometimes spaces are trimmed to a single one. Sometimes tabs are replaced by 4 spaces. There are so many use-cases and each formatter handles it differently depending on the location and type of the comment. I can create a list of some examples why it's so (maybe too) hard to please every formatter.



Any proposal how to add support for this in check ? to let skip such trailing comments.

Comments are handled differently in every formatter and within each formatter itself. Maybe an option like the following will be fine.
Separate comments by
  • ignore
  • single_space
  • single_whitespace
  • multiple_spaces
  • multiple_whitespaces

Roman Ivanov

unread,
Oct 14, 2015, 8:46:13 AM10/14/15
to sevntu-c...@googlegroups.com
Hi Robert, 

> 100% now :D

please always send me link to latest changes

> Tokens should be separated by a single space character. A tab between tokens may look like the following example and will be replaced by a formatter anyway.

it is ok, but Tab is single character, so you have to describe this nuance in javadoc.

> Separate comments by

I do not understand this description, please extend, examples helps a lot. In Javadoc we will need to describe why that option exists (it is not that obvious)

>single_space
single_whitespace

Not clear difference - Please explain.

----------------------

1) did you send/discuss other changes to guava project ? can authors accept them ?

2) Looks like you Check even in most simple implementation is controversial, I recommend to run it against Spring project . After that we could decide on what state we will merge your Check to project and make first release of it.

thanks,
Roman Ivanov
​

Robert Whitebit

unread,
Oct 14, 2015, 10:31:37 AM10/14/15
to Sevntu Checkstyle
please always send me link to latest changes

Sure, Change-set for the PR
As you can see, I just swapped the statements in the return. Instead of A && B, I had to use B && A. Otherwise it's impossible to get 100% code coverage.

single_space
single_whitespace

Not clear difference - Please explain.

single_space: A comment has to be preceded by a single space character (" ")
single_whitespace: A comment has to be preceded by a single whitespace character (space, tab or any other whitespace character). Just to allow cases like https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/db1e1201d1aa80ad1fd174fd4ca323b8fa79de2d/sevntu-checks/src/test/resources/com/github/sevntu/checkstyle/checks/coding/InputRedundantReturn.java#L117  and you don't want to allow multiple_whitespaces.
 
1) did you send/discuss other changes to guava project ? can authors accept them ?

No, what other changes do you have in mind?

2) Looks like you Check even in most simple implementation is controversial, I recommend to run it against Spring project . After that we could decide on what state we will merge your Check to project and make first release of it.

Result of the Spring project. (guava and sevntu.checkstyle)

Maybe we should drop(ignore) comments entirely for the first release?
Comments and cases you posted previously needs special treatment.

I changed the return value of the method isCommentNodesRequired from true to false and uploaded the results. This will ignore comments entirely. (except block comments which is embed in code.
int /* private */ a;
The check will report an error if */ and a is not separated correctly. A fix is easy, but I'll wait for feedback first :) )


lg :)

Roman Ivanov

unread,
Oct 14, 2015, 3:53:31 PM10/14/15
to sevntu-c...@googlegroups.com
Hi Robert,

1) as you got 100% coverage you do not need update in pom.xml (that lines are fr old Checks only), please remove your changes.


2) Please squash all commits in one and do PR to our project, we a re close to merge event.

3)
> No, what other changes do you have in mind?

almost all violations that you found in Guava we might to propose to Guava. If merged - your Check is useful. If not - we will get another nuance with spaces and explanation :).
Real testing of your Check.

4) 
> Maybe we should drop(ignore) comments entirely for the first release?

Reasonable, but what if will make it by user controlled option ? boolean validateCommentNodes


5)
>  This will ignore comments entirely. (except block comments which is embed in code

Add this as limitation to javadoc and description of the Check.



I will do review of reports a bit later..... But can you provide a analysis and summary of violations in Spring and Guava ?

thanks,
Roman Ivanov
​

Robert Whitebit

unread,
Oct 18, 2015, 1:06:19 PM10/18/15
to Sevntu Checkstyle
Hi! :) 

1) as you got 100% coverage you do not need update in pom.xml (that lines are fr old Checks only), please remove your changes.
2) Please squash all commits in one and do PR to our project, we a re close to merge event.

1+2 Done

3)
> No, what other changes do you have in mind?

almost all violations that you found in Guava we might to propose to Guava. If merged - your Check is useful. If not - we will get another nuance with spaces and explanation :).
Real testing of your Check.

See my answer to "analysis and summary of violations in Spring and Guava"
 
4) 
> Maybe we should drop(ignore) comments entirely for the first release?

Reasonable, but what if will make it by user controlled option ? boolean validateCommentNodes

Done ;)

5)
>  This will ignore comments entirely. (except block comments which is embed in code

Add this as limitation to javadoc and description of the Check.

I will add a detailed javadoc in the next days and also do all missing steps you posted earlier.

I will do review of reports a bit later..... But can you provide a analysis and summary of violations in Spring and Guava ?

I inspected all violations in the guava project. Since the check currently can't handle horizontal alignment, it can't be used in the project. Mostly all violations are caused by horizontal alignment.

I inspected the first 100 violations in the spring project precisely and the remaining violations just sporadically. Since it doesn't use horizontal alignment, all violations can be fixed and the check can be used. So, this project would benefit from such a check.

I'm not sure which IDE formatters currently can handle horizontal alignment. Also there are pros and cons https://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-alignment

Roman Ivanov

unread,
Oct 23, 2015, 9:35:50 AM10/23/15
to sevntu-c...@googlegroups.com
Hi Robert,

please always(in each post) send me link to latest commit and reports for review.
I am participating in about 10-20 code-reviews in the same time, it is hard too search for required commit again and again. It also prove that you re-pushed changes.

> Mostly all violations are caused by horizontal alignment.

Can you send them PR with changes for non-horizontal alignment cases ? It will be just real example of how useful your Check.

> I inspected the first 100 violations in the spring project precisely and the remaining violations just sporadically.

I recommend to send PR to them for 50-100 cases ,to see feedback.

thanks,
Roman Ivanov
​

Robert Whitebit

unread,
Oct 23, 2015, 10:27:05 AM10/23/15
to Sevntu Checkstyle
Hi! :)

Awesome timing! I just added some comments and missing descriptions as you mentioned in https://groups.google.com/d/msg/sevntu-checkstyle/kSCClhhf_hs/EUf8NmWcCQAJ @Nr. 2

I haven't tried yet if the plugins work now... (have to figure out how to do that first ^^)
and I'll also squash every commit in a single one before I open a PR on sevntu.checkstyle.

> Mostly all violations are caused by horizontal alignment.
Can you send them PR with changes for non-horizontal alignment cases ? It will be just real example of how useful your Check.
> I inspected the first 100 violations in the spring project precisely and the remaining violations just sporadically.
I recommend to send PR to them for 50-100 cases ,to see feedback.

Will do, but I'm not happy about creating such PRs. It may seem like I just want to get credits on the projects. Since it's for a Checkstyle check, I guess it will be appreciated. However, such a PR may also create merge-conflicts in the other open PRs or any other local commits. :S

Roman Ivanov

unread,
Oct 23, 2015, 12:38:39 PM10/23/15
to sevntu-c...@googlegroups.com
Hi Robert,

1)
> Will do, but I'm not happy about creating such PRs.

I did not mean that you has to advertise your implementation, please PR  as general update to make code looks good, as all of us debugging their code from time to time.

2)
> However, such a PR may also create merge-conflicts in the other open PRs or any other local commits.

never afraid of this. With such logic nobody should do any changes. if PR is not possible to rebase, it is goood sign that that PR should be split into few.

3)
> Maybe an option like the following will be fine. Separate comments by 

What is our resolution for this ? Do you want to make first release without this option ?



4) Please send me link to latest reports for last verification.

thanks,
Roman Ivanov
​

Robert Whitebit

unread,
Oct 23, 2015, 3:27:18 PM10/23/15
to Sevntu Checkstyle
 
3)
> Maybe an option like the following will be fine. Separate comments by 
What is our resolution for this ? Do you want to make first release without this option ?
 
Imo your solution (by using the flag validateCommentNodes) is currently better. ;)
Horizontal alignment (code + comments) are improvements for further releases. I'm also not sure if I'm skilled enough to do this. :S


Roman Ivanov

unread,
Oct 24, 2015, 8:50:10 AM10/24/15
to sevntu-c...@googlegroups.com
Hi Robert,

can you do PR to our project ? 

few additional minors :

>public void setValidateCommentNodes(boolean validateCommentNodes) {

please recheck why indentation is wrong.

> private boolean validateCommentNodes = true;

make it false by default. Looks like Spring also use this rule to separate trailing comment by double space.

> Spring PR

answer to question in PR :)

> Guava PR

you can close it, google have its own formatter - good to know.


thanks,
Roman Ivanov
​

Robert Whitebit

unread,
Oct 25, 2015, 6:25:06 PM10/25/15
to Sevntu Checkstyle
can you do PR to our project ?

 
>public void setValidateCommentNodes(boolean validateCommentNodes) {
please recheck why indentation is wrong.

 
> private boolean validateCommentNodes = true;
make it false by default. Looks like Spring also use this rule to separate trailing comment by double space.

 
> Spring PR
answer to question in PR :)

Oh, nice! ^^
 
> Guava PR
you can close it, google have its own formatter - good to know.

Closed

Let me know if there is more to do for this PR :)

Thanks a lot for your support and guidance!

Roman Ivanov

unread,
Oct 25, 2015, 9:18:59 PM10/25/15
to sevntu-c...@googlegroups.com
Hi Robet,

released as 1.16.0, thanks for contribution.

http://sevntu-checkstyle.github.io/sevntu.checkstyle/#1.16.0
​
thanks,
Roman Ivanov
Reply all
Reply to author
Forward
0 new messages