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

IllegalTokenText in google_checks should not has BACKSPACE character #3701

Closed
romani opened this Issue Jan 8, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@romani
Member

romani commented Jan 8, 2017

from #3700

problem is misconfiguration -

<property name="format" value="\\u00(08|09|0(a|A)|0(c|C)|0(d|D)|22|27|5(C|c))|\\(0(10|11|12|14|15|42|47)|134)"/>

Unicode Character 'BACKSPACE' (U+0008)/index.htm should be removed from this list.

/tmp $ javac Demo.java

/tmp $ cat Demo.java
public class Demo {
  public static void main(String[] args) {
    System.out.println("\u0008");
  }
}

$ java -cp checkstyle-7.4-all.jar com.puppycrawl.tools.checkstyle.Main -c /google_checks.xml Demo.java 
Starting audit...
[WARN] /tmp/Demo.java:3: Unicode escape(s) usage should be avoided. [AvoidEscapedUnicodeCharacters]
[WARN] /tmp/Demo.java:3:24: Avoid using corresponding octal or Unicode escape. [IllegalTokenText]
Audit done.

mapping of google style to checkstyle rule - http://checkstyle.sourceforge.net/google_style.html#a2.3.2

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 8, 2017

Member

@dimo414 , please be welcome with PR.

build system will force you to change https://github.com/checkstyle/checkstyle/blob/master/src/it/resources/com/google/checkstyle/test/chapter2filebasic/rule232specialescape/InputIllegalTokenText.java , please place "// ok, as it is backspace symbol" comment left to all "\u0008"

Member

romani commented Jan 8, 2017

@dimo414 , please be welcome with PR.

build system will force you to change https://github.com/checkstyle/checkstyle/blob/master/src/it/resources/com/google/checkstyle/test/chapter2filebasic/rule232specialescape/InputIllegalTokenText.java , please place "// ok, as it is backspace symbol" comment left to all "\u0008"

@dimo414

This comment has been minimized.

Show comment
Hide comment
@dimo414

dimo414 Jan 9, 2017

Contributor

@romani I'd also like to improve the error message (since I don't understand what the current one is telling me). What is this check actually trying to prevent, so that I can propose a more helpful fix? "Illegal text" isn't very informative :)

Contributor

dimo414 commented Jan 9, 2017

@romani I'd also like to improve the error message (since I don't understand what the current one is telling me). What is this check actually trying to prevent, so that I can propose a more helpful fix? "Illegal text" isn't very informative :)

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 9, 2017

Member

Please propose better wording.
It could be fixed in 2 ways:

Most likely you need first approach.

Member

romani commented Jan 9, 2017

Please propose better wording.
It could be fixed in 2 ways:

Most likely you need first approach.

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 13, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 13, 2017

Member

@dimo414 Any suggestions for the new message wording?

Member

rnveach commented Feb 13, 2017

@dimo414 Any suggestions for the new message wording?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 15, 2017

Member

@rnveach , if no reply in 1 week, please feel free to update message to what you think is reasonable.

Member

romani commented Feb 15, 2017

@rnveach , if no reply in 1 week, please feel free to update message to what you think is reasonable.

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 16, 2017

romani added a commit that referenced this issue Feb 16, 2017

@romani romani added the bug label Feb 16, 2017

@romani romani added this to the 7.6 milestone Feb 16, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 16, 2017

Member

fix is merged.

Member

romani commented Feb 16, 2017

fix is merged.

@romani romani closed this Feb 16, 2017

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