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

Cached Google Style is slightly outdated and not like original #6619

Closed
Vampire opened this issue Mar 30, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@Vampire
Copy link
Contributor

commented Mar 30, 2019

If you compare http://google.github.io/styleguide/javaguide.html and https://checkstyle.org/styleguides/google-java-style-20170228.html, you that it is displayed differently, for example original
grafik
vs. cached
grafik

Also the cached version got some minor changes that as far as I have seen do not change meaning, just add some clarification, as can be seen here: https://​github.com/google/styleguide/compare/594d91bfbb8dfcbcac0f148af8175f2b7ec9857b...91d6e367e384b0d8aaaf7ce95029514fcdf38651#diff-b6c9191ee5ccb7a5ef170e98e51d94d8

I'd like to suggest to update the cached version with a recent one that contains latest changes and also displays correctly.

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 30, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 30, 2019

@rnveach

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

google/styleguide@594d91b...91d6e367e384b0d8aaaf7ce95029514fcdf38651diff-b6c9191ee5ccb7a5ef170e98e51d94d8

The link says "There isn’t anything to compare.".

as I have seen do not change meaning, just add some clarification

Nothing is usually ever as simple with the google style. We would have to review every change and agree that nothing needs to be changed on our side for each module. We have worked hard to do our best with matching their guide, so we need to continue doing it with each update.

Here is possibly one example I am seeing from their webpage:

4.4 Column limit: 100
A "character" means any Unicode code point.
Each Unicode code point counts as one character, even if its display width is greater or less. For example, if using fullwidth characters, you may choose to wrap the line earlier than where this rule strictly requires.

We don't support this as seen with #5089 .

So we can't update our cache until we are sure we support all google's rules or possibly agree that we can't ever support them. Example: http://checkstyle.sourceforge.net/google_style.html#a5.2.4

@romani

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

So we can't update our cache until we are sure we support all google's rules or possibly agree that we can't ever support them.

we can update cached version, only if there is no java style changes but there is some formatting changes in HTML layout so to look better.

If there is some style changes, we have to address them in our mapping page, while copy new version of style guide.
We can update our mapping page with links to our issues to show user that paragraph is not covered till, or paragraph is not completely covered and here a list of issues to resolve to make it complete.
If paragraph just have clarificaton we need to make sure we have this in our Integration Tests input base.


If you udpate this issue description with clear definition of style differences I can approve the issue.

I see diff as https://www.diffchecker.com/KVRGxmti (if link does not work, copy html content from both pages )

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

The link says "There isn’t anything to compare.".

Stupid GitHub-style markdown parser destroyed the link by removing the # that separated the URL from the anchor.
Link is fixed now, please look again.

Nothing is usually ever as simple with the google style. We would have to review every change and agree that nothing needs to be changed on our side for each module. We have worked hard to do our best with matching their guide, so we need to continue doing it with each update.

Of course, that's why I posted the link with the differences that now also works.

But I can of course post them here again as images for reference:

4.4 grafik
4.6.1 grafik
4.6.2 grafik
4.8.2.1 grafik
5.1 grafik
5.2.3 grafik
5.2.4 grafik

4.4: As mentioned by @rnveach, #5089 shows this is false, but I would tend to say this is simply a bug and even in the current cached version there should be no green checkmark. The additional text just explains what was meant by the word character, but it was no content change, the rule is the same and is not followed correctly currently by checkstyle.

4.6.1: imho just nicer language, no content change

4.6.2: This one I actually overlooked initially. I think there is no rule yet to check these two? Actually for the ellipsis case there is even a rule in the checks that checks the opposite. (NoWhitespaceBefore with token type ELLIPSIS) For the array initializer there would also be an opposing rule (NoWhitespaceAfter with token type ARRAY_DECLARATOR which checks before is no space? wtf? It is not in the google checks, but why is this token tpye not handled in NoWhitespaceBefore instead? o_O)

4.8.2.1: clarifcation that is already tested like that: com.google.checkstyle.test.chapter4formatting.rule4821onevariableperline.InputMultipleVariableDeclarations#method2
grafik

5.1: imho just nicer language, no content change

5.2.3: mainly nicer language and not fully covered regarding the clarified point anyway

5.2.4: Not covered anyway, change is just single underscore forced

If there is some style changes, we have to address them in our mapping page, while copy new version of style guide.

Of course, as mentioned originally I thought there were only lingo changes and clarifications, while one is discutable and one I just missed.

I see diff as https://www.diffchecker.com/KVRGxmti (if link does not work, copy html content from both pages )

Not really necessary to use some differ, as the style is maintained in a GitHub repository. :-)

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

I added in my local branch the mention of #5089 for 4.4 which is only partly covered anyway and changed 4.6.2 to partly covered with explanation about the new cases. Actually 4.6.2 is only partly covered anyway, for example it is not checked that there is no space before ARRAY_DECLARATOR and probably other cases too.

I'll push it as soon as we agreed and you reopened my PR.

@romani

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

4.4

we have a bug on unicode symbol usage and LineLength from google people, we need to mention it in mapping.

4.6.1

agree, no functional changes.

4.6.2

ok, lets convert to partly covered, and create issue that address certain items to update.


the rest items are fine.


@rnveach , I am agree to change our style guide, only if we clearly define what new items we do not cover and provide links to issues. We just need to honest with users.
If you agree ... lets make issue approved and reopen PR.

@Vampire , thanks a lot for analysis but lets make sure that in new coverage change image on item if we do not cover it completely and reference issues in mapping table that need to be fixed to make full coverage or improve coverage.

@romani romani assigned rnveach and unassigned romani Apr 17, 2019

@rnveach rnveach removed their assignment Apr 17, 2019

@rnveach rnveach added the approved label Apr 17, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue May 1, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue May 1, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue May 7, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue May 7, 2019

@romani romani closed this in #6620 May 7, 2019

romani added a commit that referenced this issue May 7, 2019

@romani romani added this to the 8.21 milestone May 7, 2019

@romani romani added the miscellaneous label May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.