Maxim Vetrenko - Checkstyle configuration for Google's Java Style guide - phase 2

[Макс Ветренко]

Daily: 

1, Update NoLineWrap (1h) 

2. Update xdoc for OneTopLevelClass. Full rewrite OneTopLevelClass. (1.5h)

3. Implement update for DeclarationOrder (1.5h)

4. Investigate and starting to implement WhitespaceAround. (1h) 

5. Read FAQ about mid-term evaluations. (40m) 

6. Investigate UTF8 to prepare implementation Utf8EncodingCheck. (1h)

Total time ~6.5h 

[Roman Ivanov]

Hi Max,

>  Total time ~6.5h

I do not see work on testing project, we still behind the plan, why working hours are less then 8h ?

thanks,

Roman Ivanov

[Макс Ветренко]

Daily:

1. Send a lot of time at WhitespaceAround Update (4-5h).

2. Testing new Checks, found a false-positive it OneTopLevelClass.(~2h) 

3. Fixed false-positive in OneTopLevelClass (40m), I don't finish it yet, but I can provide a commit. 

[Roman Ivanov]

Hi Max,

please put this to you plan for development:

https://sourceforge.net/p/checkstyle/feature-requests/567/

Current Check does not cover all cases, and it is required in Google Style too.

thanks,

Roman Ivanov

[Ruslan Diachenko]

Max,

Provide all squashed commits which are ready for review.

[Макс Ветренко]

Roman:

1. Done

Ruslan:

 Please, make a code review:

1. https://github.com/maxvetrenko/checkstyle/commit/f2e9f4d7df9b452c55156a252f948d157ea8bf82

2. https://github.com/maxvetrenko/checkstyle/commit/b70793222b93e024789241241132d2ab38c90346

3. https://github.com/maxvetrenko/checkstyle/commit/3abac713351042355b744c87c3c77c45c20cd686

4. https://github.com/maxvetrenko/checkstyle/commit/06a8db6da99cf5c5afec41e23b390f87e98dc9a1

prepare input for test project 

[Макс Ветренко]

Code review points 1 and 2 look here:

1. https://github.com/maxvetrenko/checkstyle/commit/7a3a5e7256ca71a2e108d9e4924599d72f78759d

2. https://github.com/maxvetrenko/checkstyle/commit/9bfb189782abe31925caab38a42d76acc479da24

[Ruslan Diachenko]

1. NoLineWrap - reviewed

2. Update for existing Check: WhitespaceAround - reviewed

3. Update for existing Check: DeclarationOrder - reviewed

See my comments in the issues.

[Ruslan Diachenko]

Max,

Where is your daily report for Jun 24th ??

[Макс Ветренко]

Sorry for dialy report, today I'll wirte it

Prepare 4 commits for another code review:

1. https://github.com/maxvetrenko/checkstyle/commit/dee85970604ea344de2fdb09b446ea51322f6a5b

2. https://github.com/maxvetrenko/checkstyle/commit/a4ad8b23ae17f1f650313d59cc5d4136eeaf31a3

3. https://github.com/maxvetrenko/checkstyle/commit/7a3a5e7256ca71a2e108d9e4924599d72f78759d

4. https://github.com/maxvetrenko/checkstyle/commit/105c4fda35bb259e473c571fd2760e7b092ead2a

[Макс Ветренко]

Daily report. Can't provide temporal expenditures because I don't exatcly how much time have i spend. But it's more then 7 h

1. Prepare commits to next code review. Links porvide above. Please, make code review. Have a two questions https://github.com/maxvetrenko/checkstyle/issues/22, https://github.com/maxvetrenko/checkstyle/issues/20 

2. Worked at test project but I have faild. Roman, now, I don't know how to resolve. I'll try tonight again, but I'm not sure about the result. 

[Ruslan Diachenko]

Max,

I can't catch you to discuss your questions. Please call me if you're able

[Ruslan Diachenko]

1. reviewed

2. reviewed

3. reviewed

4. reviewed

>> Have a two questions https://github.com/maxvetrenko/checkstyle/issues/22, https://github.com/maxvetrenko/checkstyle/issues/20 

See answers in my new review notes in the issues comments

[Ruslan Diachenko]

>> Can't provide temporal expenditures because I don't exatcly how much time have i spend. But it's more then 7 h

I know you can! You've already done it. Please provide at least approximate time for each point you mention in your daily reports

[Макс Ветренко]

Daily:

please make code review for pull request: (1.5 h)

1. https://github.com/maxvetrenko/checkstyle/commit/36702aab3913313e55b04471af7bf0f54208cb37

2. https://github.com/maxvetrenko/checkstyle/commit/4a945896965c54152d95ca86d76ec61fb95e0193

3. https://github.com/maxvetrenko/checkstyle/commit/81bce3b5b08ba9217d2c01779708877a3b321bc8

Lose a lot of time with test project (~6.5 h)

[Макс Ветренко]

Daily:

1. Ready 

    a. https://github.com/maxvetrenko/checkstyle/commit/36702aab3913313e55b04471af7bf0f54208cb37

    b. https://github.com/maxvetrenko/checkstyle/commit/4a945896965c54152d95ca86d76ec61fb95e0193

    c. https://github.com/maxvetrenko/checkstyle/commit/81bce3b5b08ba9217d2c01779708877a3b321bc8

    d .https://github.com/maxvetrenko/checkstyle/commit/577a1f7657ae194a04dbd8b27daa707093a27ed2 (40m)

    previous commit are ready for code review. Updated JavaDoc, xdocs. fixed checkstyle warnings. 

2. Created UT with run Checkstyle on source files, Take properties from xml file. Commit (>8h) Yes, I really spent a lot of time on it. But now it woks. 

3. Starting work with SeparatorWrap (30m). 

Total time: ~9,5h

[Roman Ivanov]

Hi Max and Ruslan,

I marked as confirmed coverage of rule 4.1.3 .

After long discussion with Dima Voytenko , he persuaded me that there is not conflict in requirements, 4.1.3 extend 4.1.1.

Details with examples - http://roman-ivanov.blogspot.com/2014/06/google-java-style-mandatory-braces-rules.html

Max, you need to update this Check to let force non-empty block only for multi-block sentences. 

So 

If (a==1) {} //is not OK 

If (doSideEffect()==1) {} //is not OK, 

while ((r = in.read()) != 0) {} // is OK 

for (; index < s.length() && s[index] != 'x'; index++) {} // is OK

and 

If (a==1) {} else {print(a)} // is not OK 

Till comments support are not in Checkstyle you can not extend this check to allow empty blocks that have a comments.  

 // it is OK, but you can not support this now

If (a==1) {/*it is ok */} else {print(a)}

thanks,

Roman ivanov

[Roman Ivanov]

Max,

1) update wiki for "4.1.3 Empty blocks: may be concise EmptyBlock"

2) use formatter for your code https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/CheckstyleFormatterForEclipse.xml

make sure that eclipse do formatting automatically on each Cntl+S

3) uberi "ConfigurationBuilder builder = new ConfigurationBuilder(new File("src/"), "checkstyle_google_style.xml");" v metod @BeforeClass

4) >>> verify(builder.getCheckConfig("FallThrough"), builder.getFilePath("InputFallThrough"), expected);

[07:58:47] RomanIvanov: vynesi v peremennye vse argumenty

5) 

 "14:13: Fall through from previous branch of the switch statement.",

         "38:13: Fall through from previous branch of the switch statement.",

Remove that copy paste, use the same approach as in sevntu-checkstyle project.

thanks,

Roman Ivanov

 

[Ruslan Diachenko]

Max,

1. a, b, c, d - reviewed

See my comments in the issues

[Макс Ветренко]

Daily:

Ruslan, fix your comments, attached commits look at issues. (1h)

Worked at Separators. Investigated separators, worked at commit (1h)

Worked at testproject. 

Changed project structure (1.5h)

Create and debug CunfigurationBuilder class to collect all methods to work with xml (4h)

Create example test. It works if use String expected messages instead of getCheckMessage. No it throws NPE. Worked at it, tomorrow I'll tried resolve it. (2h)

Roman:

1. Done

2. Has not yet been made

3. Done

4. Done

5. Have an NPE in getCheckMessage, in progress. (all 30m)

Total time: ~10h

[Макс Ветренко]

Ruslan, fix your comments, attached commits look at issues

Sorry, >> fixed your comments 

[Макс Ветренко]

Ruslan, fixed mistakes. Please, check it:

1. https://github.com/maxvetrenko/checkstyle/commit/8ed32673de0f7aa8f7aab4f035e7588df23a4cd2

2. https://github.com/maxvetrenko/checkstyle/commit/0c5258c5723dfc113d0ce9204c922b16775d6766

3. https://github.com/maxvetrenko/checkstyle/commit/a926866e769347094caf1fb3713f854c53d2e4d2

4. https://github.com/maxvetrenko/checkstyle/commit/7341a9eb7b1798e32fb8fdf7cee9e21a77ba3093

[Ruslan Diachenko]

1 - reviewed, see my notes in the issue

2 - reviewed, see my notes in the issue

3 - reviewed, no more questions. Roman please have a look at it if I missed something.

4 - reviewed, see my notes in the issue

[Макс Ветренко]

Ruslan:

1. https://github.com/maxvetrenko/checkstyle/commit/3ac34535dbcc00af3143014f0d3c76fa7d02711a

2. https://github.com/maxvetrenko/checkstyle/commit/e510af09e743c21d42000d0fd04b8f89421ae8d1

3. https://github.com/maxvetrenko/checkstyle/commit/c4043e400327a87a7ef67f86651bc01f467fc1cc

[Макс Ветренко]

Daily:

1. prepare Checks for code review. (30m)

2. New Check SeparatorWrap (1.5h)

3. Working at NPE in this test. Didn't resolve. (4h)

[Roman Ivanov]

Hi Max,

> Roman please have a look at it if I missed something.

done, comments are in issue https://github.com/maxvetrenko/checkstyle/issues/4

thanks,

Roman Ivanov

[Макс Ветренко]

Roman:

1. https://github.com/maxvetrenko/checkstyle/commit/fca113948d6c436f79a2288ca04a0b46eb61acfb

2. https://github.com/maxvetrenko/checkstyle/commit/29cc04788ac40d377fb4389b1b08c3b59c83592b

3. https://github.com/maxvetrenko/checkstyle/commit/dcf5a852240fde6431267107f13914f5c079cf53

4. https://github.com/maxvetrenko/checkstyle/commit/5eee506cf324b7bb7b6e2ade19b6c94f2b910a43

Done

[Макс Ветренко]

Introduce new Check SeparatorWrap. Found and fixed false-positive in following case: s.isEmpty();

Please, make code review. 

[Макс Ветренко]

Sorry, after testing on Guava I found false-positive and fixed them. New commit SeparatorWrap

[Ruslan Diachenko]

No more questions to:

1. https://github.com/maxvetrenko/checkstyle/issues/22

2. https://github.com/maxvetrenko/checkstyle/issues/21

3. https://github.com/maxvetrenko/checkstyle/issues/20

Roman please have a look at them in case I missed something

[Roman Ivanov]

Hi Ruslan and Max,

> Roman please have a look at them in case I missed something

I put comments to all issues.

thanks,

Roman Ivanov

​

[Ruslan Diachenko]

Max,

Reviewed, see my comments in the issue

[Макс Ветренко]

Dailly:

Roman:

1. https://github.com/maxvetrenko/checkstyle/commit/c6a53c4aa44ad2258f9b7f22c09056ec4e7d7fab

2. https://github.com/maxvetrenko/checkstyle/commit/6d7a27f35260808cb495436dc4af5b81a2d5c874

3. https://github.com/maxvetrenko/checkstyle/commit/280e7b323cd5b95edc79e893bd6bffd7fc0d7519

(1h)

New Check: SeparatorWrap. Found bugs, tested on Guava. Check has a 100% coverage. Found bug in ANTLR. 
Grammar can't detect "|" in folowing case:

catch (FooException  | BarException e) {} (4h)

Ruslan I tried to implement another logic, but I could not found another fast and simple solution. 

Roman, I tried to fix NPE in test project, I spent again 3 hours, but I couldn't do it. I need help, I spent a lot of time but solution is still not found. 

Read about UTF8, investigate a bit "isutf8.c" from https://joeyh.name/code/moreutils/ (1h)

[Roman Ivanov]

Hi Max,

> 1. , 2. 3.  ..... - 1h

please provide links to issue , in issues pages I expect "Done" on each point that I write to you.

If my comment is last I will not take a look.

> Grammar can't detect "|" in folowing case:

What exactly is working as non expected ? create issue against Checkstyle project ans describe all in details

> Roman, I tried to fix NPE in test project, I spent again 3 hours

Did you discussed that problem with Rulsan on Jun30 ? as I requested.

> Read about UTF8, in

USE GOOGLE TRANSLATOR FOR ALL THAT YOU POST IN MAIL-LIST IN ENGLISH!!!!

Please postpone UTF-8 issue till the very end of GSoC, it is very complicated issue, and it usefulness is minimal and most likely http://jchardet.sourceforge.net/ should be used.

thanks,

Roman Ivanov.

[Roman Ivanov]

Hi Max,

> Roman, I tried to fix NPE in test project, I spent again 3 hours

required file is located at

...../checkstyle/target/checkstyle-5.8-SNAPSHOT.jar/com/puppycrawl/tools/checkstyle/checks/coding/messages.properties

so have to loaded as:

pr.load(getClass().getResourceAsStream("/com/puppycrawl/tools/checkstyle/checks/coding/messages.properties"));

or

pr.load(getClass().getClassLoader().getResourceAsStream("com/puppycrawl/tools/checkstyle/checks/coding/messages.properties"));

ATTENTION , Checkstyle have numerous property files!!!! each package have its own file.

in Checkstyle "getClass().getResourceAsStream("messages.properties")" work as XXXXXCheck and property file located in the same package

to make it work you can use:

pr.load(FallThroughCheck.class.getResourceAsStream("messages.properties"));

so to ease development , and have universal load method, you need to specify class of Check, each time, example:

String msg = getCheckMessage(FallThroughCheck.class, "fall.through");

thanks,

Roman Ivanov

[Макс Ветренко]

Roman:

[Макс Ветренко]

Daily:
1. I have prepared issues. Make code review please (1h)

2. I have worked with test project. Commit. (5h)

3. I have worked with RightCurlyUpdate. (2h)

Sorry, today I feel bad. Tomorrow I'll work better. 

[Roman Ivanov]

Hi Max and Ruslan,

To Ruslan and Amx: I cleared mentors verification signs at

https://github.com/maxvetrenko/checkstyle/wiki/Google%27s-Java-Style-Checkstyle-Coverage

Mentors have to verify implementation and configuration of Checks that cover Google rules. That should be done on final stage of GSoC practice.

Max, if you change config of already verified configuration for particular Check - please leave a comment at wiki, as verification should be done again by mentors.

To Max,

https://github.com/maxvetrenko/checkstyle/issues/22 - do final changes and do PR.

https://github.com/maxvetrenko/checkstyle/issues/21 - read attentively !!!!

https://github.com/maxvetrenko/checkstyle/issues/20 - nothing was done by you.

thanks,

Roman Ivanov

[Макс Ветренко]

1. PR 

[Ruslan Diachenko]

Max,

https://github.com/maxvetrenko/checkstyle/issues/10 - reviewed, look at my comment in the issue

[Макс Ветренко]

Daily report: 

1. Ruslan:

>> SeparatorWrapCheck is for separators only, so it should contain only separators. We already have one for operators - OperatorWrapCheck. Please update it with missed operators.

Done: commit (1h)

2. Roman:

Your summary:

1. OneTopLevelClass . I corrected Javadoc, but I don't agree with you that we have to remove iterator.

2. NoLineWarp . I corrected Javadoc.

My new update: RightCurly (6h). I spent so much time at Check constantly because there are various Exceptions, especially NPE.

morning update (30m)

[Roman Ivanov]

Hi Max,

https://github.com/maxvetrenko/checkstyle/blob/8bb1cb29b775a82fe7396a8e1fa2cb8f7f4e8866/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/NoLineWrapCheck.java

> To configure the check to force no line-wrapping in package and static statements:

so please provide how to configure it in non default way. Why user need read your code to get to know how to do non standard configuration ?

https://github.com/maxvetrenko/checkstyle/blob/64752305c3d55923ddc60d4fe9429f4ea50091a8/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/OneTopLevelClassCheck.java

>  I corrected Javadoc, but I don't agree with you that we have to remove iterator.

    public void finishTree(DetailAST aRootAST)

    {

        if (!mPublicTypeFound && iterator.hasNext()) {

            // skip first top-level type.

        mLineNumberTypeMap.remove(mLineNumberTypeMap.firstKey());

        }

        for (Map.Entry<Integer, String> entry : trmap.entrySet()) {

        log(entry.getKey(), "one.top.level.class", entry.getValue());

}

        mLineNumberTypeMap.clear();

        mPublicTypeFound = false;

    }

thanks,

Roman Ivanov
​

[Макс Ветренко]

Hi, Roman:

1. Done https://github.com/maxvetrenko/checkstyle/commit/f57617f371040570fc363e3aa46ebbdc54805c82

2. Done 

https://github.com/maxvetrenko/checkstyle/blob/8eaded90d0b4ff8c4b4198928e9d08c3efe1ff4f/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/OneTopLevelClassCheck.java#L129

[Ruslan Diachenko]

Max,

https://github.com/maxvetrenko/checkstyle/issues/10 - new notes are waiting for you, please fix them

[Макс Ветренко]

Ruslan: https://github.com/maxvetrenko/checkstyle/issues/10#issuecomment-47811928

[Roman Ivanov]

Hi Max,

1.

https://github.com/maxvetrenko/checkstyle/commit/f57617f371040570fc363e3aa46ebbdc54805c82

+ * <pre class="body">

+ * &lt;module name=&quot;NoLineWrap&quot;&gt;

+ *     &lt;property name="tokens" value="IMPORT"&gt;

+ * &lt;/module&gt;

+ * </pre>

looks like only imports should be corrected but in example package and import changed.

2. 

https://github.com/maxvetrenko/checkstyle/blob/8eaded90d0b4ff8c4b4198928e9d08c3efe1ff4f/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/OneTopLevelClassCheck.java#L126

        final Iterator<Map.Entry<Integer, String>> iterator =
                mLineNumberTypeMap.entrySet().iterator();


        if (!mPublicTypeFound && iterator.hasNext()) {

Iterator only to calculate size ? what about isEmpty()

http://www.tutorialspoint.com/java/util/hashmap_isempty.htm

thanks,

Roman Ivanov
​

[Ruslan Diachenko]

Max, where is your yesterday's daily report? Please fill in your Google calendar with a "daily report" event and set up reminders. I want to see your reports each day until 9 p.m. (Kiev time). If you don't publish your report until this time it'll mean that you didn't work at all during the day.

Also what is your progress with this issue: https://github.com/maxvetrenko/checkstyle/issues/25 ? You're working on it for a quite long period of time. Did you face some problems?

[Ruslan Diachenko]

https://github.com/maxvetrenko/checkstyle/issues/10 - see my notes

On Friday, July 4, 2014 3:01:05 AM UTC+3, Макс Ветренко wrote:

Ruslan: https://github.com/maxvetrenko/checkstyle/issues/10#issuecomment-47811928

[Макс Ветренко]

Roman:

>> looks like only imports should be corrected but in example package 

[NoLineWrapCheck](https://github.com/maxvetrenko/checkstyle/blob/38c84764ada8f3f79192e8d817bda7a70186528e/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/NoLineWrapCheck.java#L60)

example was removed

>>Iterator only to calculate size ? what about isEmpty()

[OneTopLevelClassCheck](https://github.com/maxvetrenko/checkstyle/blob/e973a61d390e22411168aa84552b93f11817d72d/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/OneTopLevelClassCheck.java#L125)

iterator was removed

Ruslan:

>>I want to see your reports each day until 9 p.m. (Kiev time)

Agreed. I will write a report every day at 9 (Kiev time)

>>Also what is your progress with this issue: https://github.com/maxvetrenko/checkstyle/issues/25 ?

I'll do my best to solve problems today and close this issue. 

>>https://github.com/maxvetrenko/checkstyle/issues/10 - see my notes

https://github.com/maxvetrenko/checkstyle/issues/10

[Макс Ветренко]

RightCurlyUpdate is ready for code review. 

[Макс Ветренко]

Skype summary with Roman:

http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s5.2.7-local-variable-names

"However, one-character names should be avoided, except for temporary and looping variables.". It's impossible to detect temporary variables. So, we'll detect only looping variables in for loops. 

[Макс Ветренко]

Daily report:

1. RightCurlyUpdate is ready for code review. (3h)

2. Resolving problems with another checks(1h)

3. Worked on the issue Update for existing Check: LocalVariableNameCheck https://github.com/maxvetrenko/checkstyle/issues/23 (2h)

I'll continue the work tonight and try to finish RightCurlyUpdate 

[Ruslan Diachenko]

Max,

Please add your comment to the issue

[Ruslan Diachenko]

Max,

https://github.com/maxvetrenko/checkstyle/issues/10 - another bunch of notes is available

>> RightCurlyUpdate is ready for code review. 

and the next message

>> I'll continue the work tonight and try to finish RightCurlyUpdate 

Where is the truth? Notify me when this check is really ready for review

[Roman Ivanov]

Hi Max,

https://github.com/maxvetrenko/checkstyle/issues/20

https://github.com/maxvetrenko/checkstyle/issues/21

ok for PR, attention to my last comment in that issues.

thanks,

Roman Ivanov
​

[Макс Ветренко]

Ruslan:

>>Where is the truth? Notify me when this check is really ready for review

My mistake, I meant LocalVariableNameCheck.  RightCurlyUpdate is ready for code review. 

>> https://github.com/maxvetrenko/checkstyle/issues/10 

Corrected

Roman:

>>https://github.com/maxvetrenko/checkstyle/issues/20

Done

>>https://github.com/maxvetrenko/checkstyle/issues/21

Have a problem with Travic, but local building succeeds. Don't understand 

Failed tests: testFileWithSecondEnumTopLevelClass(com.puppycrawl.tools.checkstyle.checks.design.OneTopLevelClassCheckTest): unexpected output: /home/travis/build/checkstyle/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputDeclarationOrder.java:210: Top-level class Fooable has to reside in its own source file. expected:<1> but was:<2>

[Макс Ветренко]

Daily:

1. Ready: RightCurlyUpdate (1.5h)

2. Ready: LocalVariableNameUpdate (3h)

3. I tried to solve the problem with OneTopLevelClass PR, spent 3 hours, but I did't find answer. Help me

[Roman Ivanov]

Hi Max,

https://github.com/maxvetrenko/checkstyle/issues/20

https://github.com/maxvetrenko/checkstyle/issues/21

merged to main project.

> Have a problem with Travic

I reproduced problem in Eclipse, UTs was updated as it had wrong expectations.

thanks,

Roman Ivanov​

[Ruslan Diachenko]

final String text = aRcurly.getText();

final int colNo = aRcurly.getColumnNo();

final int lineNo = aRcurly.getLineNo();

final String currentLine = getLines()[lineNo - 1];

if ((currentLine.substring(0, colNo).trim().length() != 0)

        || currentLine.substring(colNo + text.

                length()).trim().length() != 0)

                

Roman and Max,

I noticed that the aforementioned code and its interpretations are already used in OperatorWrapCheck (and by Max in SeparatorWrapCheck and RightCurlyCheck) checks. It checks whether a token starts the line or finishes the line. Checkstyle already has a method for checking if a token starts the line. For instance:

final boolean startsLine = Utils.whitespaceBefore(rcurly.getColumnNo(), getLines()[rcurly.getLineNo() - 1]);

I looked through the Utils class but it doesn't contain the opposite method for checking if a token finishes the line. I propose to update Utils class with the following method:

public static boolean whitespaceAfter(int aIndex, String aLine)

It will allow us to simplify the logic in our checks and remove copy-paste by playing with both methods.

[Roman Ivanov]

Hi Ruslan,

Problem of Utils method is that it is in "api" folder.

Extending API should be avoid without big reason. Placing of Utils in API is mistake.

We will move that class out of api one day.

Right now I do not know where to put utils methods common for all Checks.

For now I recommend to keep Checks as independent as possible, even copy-paste is happening.

​

thanks,

Roman Ivanov

[Ruslan Diachenko]

Max,

1. https://github.com/maxvetrenko/checkstyle/issues/10 - reviewed, see my comments in the issue

2. https://github.com/maxvetrenko/checkstyle/issues/23 - reviewed, see my comments in the issue

3. https://github.com/maxvetrenko/checkstyle/issues/25 - reviewed, see my comments in the issue

[Ruslan Diachenko]

Max,

It seems you did nothing today as I don't see your daily report. Why?

[Макс Ветренко]

Ruslan:

1. https://github.com/maxvetrenko/checkstyle/issues/10

2. https://github.com/maxvetrenko/checkstyle/issues/23

3. https://github.com/maxvetrenko/checkstyle/issues/25

>> It seems you did nothing today as I don't see your daily report. Why?

I told you I was in Yalta on Granny's anniversary celebration. Came back very late and could't work at all. 

[Макс Ветренко]

Daily:

1. https://github.com/maxvetrenko/checkstyle/issues/10

2. https://github.com/maxvetrenko/checkstyle/issues/23

3. https://github.com/maxvetrenko/checkstyle/issues/25

Ready for code review (1.5h)

2. Updated Wiki (40m)

3. Updated checkstyle_google_style.xml(30m)

4. Updated html report(30m)

5. Working at New Check: EmptyLineSeparatorCheck (4h) 

[Макс Ветренко]

I'll continue my job tonight 

[Ruslan Diachenko]

1. https://github.com/maxvetrenko/checkstyle/issues/10 - no more questions, Roman please have a look

2. https://github.com/maxvetrenko/checkstyle/issues/23 - reviewed, Max see my comments in the issue

3. https://github.com/maxvetrenko/checkstyle/issues/25 - reviewed, Max see my comments in the issue

[Roman Ivanov]

https://github.com/maxvetrenko/checkstyle/issues/10

https://github.com/maxvetrenko/checkstyle/blob/24a51b2d3631c96f33a45fd5175162c838bdf5ee/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/SeparatorWrapCheck.java

1) Good code style , no questions :), but :( ...

2) What is a reason of this Check ? It is only for Google's COMMA and DOT rules?

Where are option for user to customize Check?

If no options - rename check to GooglesSeparatorWrapCheck

3) How user can setup its own rules for wrapping , I need  "," to be on next line?

https://github.com/sevntu-checkstyle/sevntu.checkstyle/issues/125 

4) 

 if (aAST.getType() == TokenTypes.COMMA) {

Why this Token is so special ? What if I do not care where COMMA is placed in code ?

5) 

 * <pre>
 * &lt;module name="SeparatorWrap"/&gt;
 * </pre>

No comments ..... Please, reply to this thread and describe me what is wrong with that example

6) 

 * A good line wrapping example

 * A bad line wrapping example:

Who told you what is right and what is not ?  Why you do decision without user ?

Style is unique from team to team, there is no standard style - each team decide itself what is good and what is bad.  

[Макс Ветренко]

Daily:

1. https://github.com/maxvetrenko/checkstyle/issues/23- Ruslan, I corrected your notes

2. https://github.com/maxvetrenko/checkstyle/issues/25 - Ruslan, I corrected your notes

3. https://github.com/maxvetrenko/checkstyle/issues/10- Roman, I changed logic, but don't finish JavaDoc.  Please have a look.

[Макс Ветренко]

Please, review:

1. https://github.com/maxvetrenko/checkstyle/issues/25

2. https://github.com/maxvetrenko/checkstyle/issues/23

3. https://github.com/maxvetrenko/checkstyle/issues/10

[Макс Ветренко]

New update was introduced : Update for existing Check: EmptyBlock #27

[Макс Ветренко]

Daliy:

1. I have prepared issues for review.

2. I have introduce new update: Update for existing Check: EmptyBlock #27

3. I have starting work at New Check: EmptyLineSeparatorCheck

Total spent time: 8.5 h

[Ruslan Diachenko]

Max and Roman,

1. https://github.com/maxvetrenko/checkstyle/issues/25

No more questions except the following note: method public int[] getDefaultTokens() is too long and complicated. This leads to different hacks in it. E.g. the one which Max had to do with return:

if (mForceLineBreakAfterClosingBrace && !isOneInTheLine(rcurly))

 +            {

 +                log(rcurly, "line.alone", "}");

 +            }

 +            else {

 +                return;

 +            }

 

I'll create issue in the main Checkstyle project to refactore this check.

2. https://github.com/maxvetrenko/checkstyle/issues/23

I don't like this update. The option mAllowOneCharVarInForLoop looks like a hack to me. What if a user uses one character variables not only in for loops??

3. https://github.com/maxvetrenko/checkstyle/issues/10

No more questions. I'll create another issue for refactoring a copy/paste

[Макс Ветренко]

Ruslan:

>>What if a user uses one character variables not only in for loops??

I think, in this case user can use ^[a-z][a-zA-Z0-9]*$ regexp

[Макс Ветренко]

Ruslan:

>> This leads to different hacks in it. E.g. the one which Max had to do with return:

I tried to fix and I think I got rid of the hack: RightCurlyUpdate

[Макс Ветренко]

I implemented EmptyLineSeparatorCheck for import cases: rule.

But now is not possible to cover this rule This case is incorrect and it's impossible to cover it because Checkstyle can't work with javadoc.

[Roman Ivanov]

Hi Max,

WHERE IS DAILY REPORT ?

> But now is not possible to cover this r

JavaDoc, comment it is not a empty line , empty line should be between declarations.

from Rule:

> Exception: A blank line between two consecutive fields (having no other code between them) is optional

that could be done by custom suppression of Checkstyle, see http://checkstyle.sourceforge.net/config.html#SuppressionFilter

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Max,

commented - https://github.com/maxvetrenko/checkstyle/issues/25

commented - https://github.com/maxvetrenko/checkstyle/issues/23

ok to PR - https://github.com/maxvetrenko/checkstyle/issues/10

commented - https://github.com/maxvetrenko/checkstyle/issues/27

commented - https://github.com/maxvetrenko/checkstyle/issues/13

2 Ruslan :

> I don't like this update. The option mAllowOneCharVarInForLoop looks like a hack to me. What if a user uses one character variables not only in for loops??

We are going to check variable declaration only in For-loop initialization expression. Max will extend JavaDoc to make all clear. 

​thanks,

Roman Ivanov

[Макс Ветренко]

Daily report:

1. https://github.com/maxvetrenko/checkstyle/issues/25 Done. Solved the problem by means of Check, remove bad code (1h)

2. https://github.com/maxvetrenko/checkstyle/issues/23 Done. (20m)

3. https://github.com/maxvetrenko/checkstyle/issues/27 Done. Solved the problem by means of Check, remove bad code (30h)

4. https://github.com/maxvetrenko/checkstyle/issues/13 Done. New Check was created. (3h). Please, make code review. xdocs will be updated tomorrow. 

5. https://github.com/maxvetrenko/checkstyle/issues/10 Done. PR was sent. 

6. http://checkstyle.sourceforge.net/config.html#SuppressionFilter Investigated but not fully understood (3h)

Total spent time: ~8.5h

[Roman Ivanov]

Hi Max,

commented - https://github.com/maxvetrenko/checkstyle/issues/25

commented - https://github.com/maxvetrenko/checkstyle/issues/23

commented - https://github.com/maxvetrenko/checkstyle/issues/27

commented - https://github.com/maxvetrenko/checkstyle/issues/13

thanks,

Roman Ivanov
​

[Макс Ветренко]

Hi,Roman:

 https://github.com/maxvetrenko/checkstyle/issues/25 

 https://github.com/maxvetrenko/checkstyle/issues/23

 https://github.com/maxvetrenko/checkstyle/issues/27

 https://github.com/maxvetrenko/checkstyle/issues/13

Now working at https://github.com/maxvetrenko/checkstyle/issues/14

[Roman Ivanov]

Hi Max,

ok to PR - https://github.com/maxvetrenko/checkstyle/issues/25

commented - https://github.com/maxvetrenko/checkstyle/issues/23

commented - https://github.com/maxvetrenko/checkstyle/issues/27

commented - https://github.com/maxvetrenko/checkstyle/issues/13

> Now working at

where is commit  or ....? 

> https://github.com/maxvetrenko/checkstyle/wiki/Google%27s-Java-Style-Checkstyle-Coverage

Please confirm that all "*" at "Applied to config" are all completely done and tested in your project. I and Ruslan need to start validating your configuration.

thanks,

Roman Ivanov
​

[Макс Ветренко]

Daily:

1.Worked at https://github.com/maxvetrenko/checkstyle/issues/14. 

 Investigated:

http://stackoverflow.com/questions/1673544/how-do-i-detect-unicode-characters-in-a-java-string

http://ru.wikipedia.org/wiki/%D0%AE%D0%BD%D0%B8%D0%BA%D0%BE%D0%B4

http://illegalargumentexception.blogspot.com/2009/05/java-rough-guide-to-character-encoding.html

http://docs.oracle.com/javase/tutorial/i18n/text/unicode.html

I came to detect Unicode using Regexp \\\\[u][a-fA-F0-9]{4}

I spent on it 4.5h. 

2. Worked on the previous fixes (1.5h)

I'll provide one more report in a few hours, because I have to fix my updates and RightCurly PR.

[Макс Ветренко]

Second Daily:

1. Fixed RightCurly PR

2. https://github.com/maxvetrenko/checkstyle/issues/23

3. https://github.com/maxvetrenko/checkstyle/issues/27

4. https://github.com/maxvetrenko/checkstyle/issues/13

Total time: 2h

[Макс Ветренко]

Rechecked Wiki. I think it's ready for review. 

[Roman Ivanov]

Hi Max,

1)

> Rechecked Wiki. I think it's ready for review.
​

Please make a "[*]" as a link to line config.

add one more link "tests" to the same table cell - it will open UTest in your test project, to prove that all was tested, and show where we could find how it was tested.

2)

 testproject / src / test / java / com / google / checkstyle / test / chapter2filebasic / rule21filename / Input15ExtensionsTest.java 

 testproject / src / main / java / com / google / checkstyle / test / chapter2filebasic / rule21filename / Input15Extensions.java 

Remove "Input" from JUnit class name - that prefix is only for test files.

Keep number in the middle of name only for package and Name of Junit class to point to rule.

Junit class should be named as Rule21_FilenameExtension.java - and all tests should be in one file.

Input files should be named like Junit file but "Input"  as prefix and "_1", "_2"  as suffix if you want to keep few input files.

thanks,

Roman Ivanov

[Макс Ветренко]

Hi Roman,

https://github.com/maxvetrenko/checkstyle/issues/14

Added boolean - allowIfAllCharactersEscaped. To cover cases like:

return '\ufeff' + content; // byte order mark

I detect all control chars according to http://en.wiktionary.org/wiki/Appendix:Control_characters

[Макс Ветренко]

Daily report:

1. Worked at https://github.com/maxvetrenko/checkstyle/issues/14

Added boolean - allowIfAllCharactersEscaped.

Investigated UTF control chars:

http://en.wiktionary.org/wiki/Appendix:Control_characters

http://en.wikipedia.org/wiki/C0_and_C1_control_code

https://www.cs.tut.fi/~jkorpela/chars.html#utf

(4h)

2. Worked with Wiki https://github.com/maxvetrenko/checkstyle/wiki/Google's-Java-Style-Checkstyle-Coverage

Recheck all Checks in config, added links to line config. (3h)

[Roman Ivanov]

Hi Max,

please update "RightCurly PR" as Daniil requested.

commented -  https://github.com/maxvetrenko/checkstyle/issues/23

commented -  https://github.com/maxvetrenko/checkstyle/issues/27

commented -  https://github.com/maxvetrenko/checkstyle/issues/13

thanks,

Roman Ivanov
​

[Roman Ivanov]

Hi Max,

https://github.com/maxvetrenko/checkstyle/blob/1ef73a51926c4c609fe67fee9c9aea1b22687eb2/src/main/java/com/puppycrawl/tools/checkstyle/checks/ForbidEscapedUnicodeCharactersCheck.java

1) You need JavaDoc to explain what you intended to implement

2) Where is formatting ? where spaces between fields ?

3) 

> Regexp Unicode control characters

I do ont understand why you should care about this characters ?

4) update UTs to have the same cases for Char and String. UTs should not care how it is implemented, they cover functionality.

5) I do not see any Inputs where you have multiple characters in string (at the begging, end in the middle , few unicode symbols in middle separated by normal characters ), I do not see how you support this from code.

6) 

from issue:

> I think you can do this with current abilities of Checkstyle, review other checks that work with comments - it is doable

I do not see that implementation

thanks,

Roman Ivanov
​

[Roman Ivanov]

Hi Max,

> Recheck all Checks in config, added links to line config. (3h)

https://github.com/maxvetrenko/checkstyle/wiki/Google%27s-Java-Style-Checkstyle-Coverage

Please do all cells like I did for "2.1 File name".

thanks,

Roman Ivanov
​

[Макс Ветренко]

1. Fixed PRs:

https://github.com/checkstyle/checkstyle/pull/187

https://github.com/checkstyle/checkstyle/pull/183

https://github.com/checkstyle/checkstyle/pull/180

2. https://github.com/maxvetrenko/checkstyle/issues/27

Roman:

>> I do ont understand why you should care about this characters ?

According to Google's rule it's a good style to use escapes for non-printable characters, and comment if necessary. Non-printable chars are also called control chars. 
Options:

boolean - allowIfAllCharactersEscaped. To cover cases like:
return '\ufeff' + content; // byte order mark

this option allow to use escapes for non-printable characters. '\ufeff' is only one control char.

[Макс Ветренко]

>> You need JavaDoc to explain what you intended to implement

Done

>> Please do all cells like I did for "2.1 File name".

Wiki. Fixed config links. Testproject links I'll add later. 

[Макс Ветренко]

Daily:

1. ForbidEscapedUnicodeCharactersCheck

>> update UTs to have the same cases for Char and String. UTs should not care how it is implemented, they cover functionality.

Done 

 >>  I do not see any Inputs where you have multiple characters in string

Were created inputs

2. Worked with Wiki

3. Fixed PRs.

Total time: 5h

[Макс Ветренко]

was implemented  ForbidEscapedUnicodeCharactersCheck. 

[Макс Ветренко]

Daily:

1. New Check was implemented  ForbidEscapedUnicodeCharactersCheck.  (3.5h)

2. >>  testproject / src / test / java / com / google / checkstyle / test / chapter2filebasic / rule21filename / Input15ExtensionsTest.java 

 testproject / src / main / java / com / google / checkstyle / test / chapter2filebasic / rule21filename / Input15Extensions.java 

Remove "Input" from JUnit class name - that prefix is only for test files.

Keep number in the middle of name only for package and Name of Junit class to point to rule.

Junit class should be named as Rule21_FilenameExtension.java - and all tests should be in one file.

Was updated testproject (3h)

3. Was updated Wiki. Added links to some tests. (40m)

4. Investigate issue with Indentation Check (1.5h)

5. https://github.com/maxvetrenko/checkstyle/issues/27

    https://github.com/maxvetrenko/checkstyle/issues/23

Total time: ~9h

[Roman Ivanov]

Hi Max,

commented - https://github.com/maxvetrenko/checkstyle/issues/23

commented - https://github.com/maxvetrenko/checkstyle/issues/14

ok to PR - https://github.com/maxvetrenko/checkstyle/issues/27

commented - https://github.com/checkstyle/checkstyle/pull/183

merged - https://github.com/checkstyle/checkstyle/pull/187

merged - https://github.com/checkstyle/checkstyle/pull/180

Call me in Hangout we need to close/resolve/fix all issues above tomorrow(July 16). 

> Was updated testproject (3h)

> 3. Was updated Wiki. Added links to some tests. (40m)

update for wiki and testprojects will be mentored by Ruslan, please cooperate with him.
​

> Investigate issue with Indentation Check (1.5h)

I recommend to postpone fixes in this Check for late, as it is not that simple. Lets fix more simple issues.

thanks,

Roman Ivanov

[Макс Ветренко]

Roman:

https://github.com/maxvetrenko/checkstyle/issues/23

https://github.com/maxvetrenko/checkstyle/issues/14

https://github.com/checkstyle/checkstyle/pull/183

[Roman Ivanov]

Hi Max,

ok to PR - https://github.com/maxvetrenko/checkstyle/issues/23

after duscussion in Skype do  update at https://github.com/maxvetrenko/checkstyle/issues/14

thanks,

Roman Ivanov

​

[Макс Ветренко]

Daily:

1. https://github.com/maxvetrenko/checkstyle/issues/14 (5.5h) 

Done

2. Prepared PRs 2h)

3. Fixed another problems (1.5)

Total: ~8.5h

[Roman Ivanov]

Hi Max,​

commented - https://github.com/maxvetrenko/checkstyle/issues/14

thanks,

Roman Ivanov