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

Issue #6619: Update cached Google Style Guide #6620

Merged
merged 2 commits into from
May 7, 2019

Conversation

Vampire
Copy link
Contributor

@Vampire Vampire commented Mar 30, 2019

Fixes #6619

@romani
Copy link
Member

romani commented Mar 30, 2019

lets finish discussion in issue first.

@romani romani closed this Mar 30, 2019
@rnveach rnveach reopened this Apr 17, 2019
Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase on the latest master and add new label to google style guide (similar to sun) on items discussed in main issue that need to still be updated.

@@ -34,7 +34,7 @@
* Comments are indented at the same level as the surrounding code.
* Detailed info about such convention can be found
* <a href=
* "https://checkstyle.org/styleguides/google-java-style-20170228.html#s4.8.6.1-block-comment-style">
* "https://checkstyle.org/styleguides/google-java-style-20180523/google.github.io/styleguide/javaguide.html#s4.8.6.1-block-comment-style">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing google-java-style-20180523.html.
I assume javaguide is the google-java-style-20180523? What is the purpose of putting in such a weird nested folder instead of where it was before?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vampire , please rebase on latest and explain extra folders.
I think you simply copied them from github repository (previously I did "page save" in browser, as it was simpler at that time, I do not remember why), so it is ok, we just need to know.

Copy link
Contributor Author

@Vampire Vampire May 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing google-java-style-20180523.html.
I assume javaguide is the google-java-style-20180523?

Exactly

What is the purpose of putting in such a weird nested folder instead of where it was before?

Previously @romani just used the browsers "Save page as" functionality, which most often does not work as intended and for example added these ugly effects I highlighted in the issue, as the page-save saves stuff that was added dynamically via JavaScript. But the JavaScript is also still there, so things are doubled. And the dynamically added images are not where expected, so you get those missing images displayed.

I used the software "WinHTTrack Website Copier" to do a proper site download that displays correctly, including all resources where they are expected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the software "WinHTTrack Website Copier"

After some time of thinking on this ...
lets not depend on some tool here, I do not know such tool, and I will not be able to use it as it is for Windows.
Please update please folders to be at google-java-style-20180523 I hope style guide html style will not be changed in recent few years. So all we will need to update periodically is html file. So some any tool, and even "Save as" will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not depend on some tool here, I do not know such tool, and I will not be able to use it as it is for Windows.

It is not.
Don't get trapped by the "Win" in the name, it is just the Windows build of HTTrack which is available for Windows, Linux, OSX and even Android: http://www.httrack.com/page/2/

Please update please folders to be at google-java-style-20180523 I hope style guide html style will not be changed in recent few years. So all we will need to update periodically is html file. So some any tool, and even "Save as" will work.

The nice thing with HTTrack is, that it properly downloads websites and all related things that are necessary for offline or cached version.
Actually in this case it is not necessary yes, because it the actual HTML that is stored in the Google repository, so we can just copy in the files.
But keep it in mind for probably future supported style guides where the source is not the actual HTML, there HTTrack or any other proper website copier is much better and more reliable than using "Save as" in the browser which will usually save JS-post-processed versions of the pages which end up unnice like it was with the Google Styleguide here too.

@rnveach rnveach self-assigned this Apr 17, 2019
@romani
Copy link
Member

romani commented Apr 27, 2019

@Vampire , your update is awesome step forward, please find time to finish it.

@Vampire
Copy link
Contributor Author

Vampire commented May 1, 2019

Please rebase on the latest master

Done

and add new label to google style guide (similar to sun) on items discussed in main issue that need to still be updated.

Done

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am approving this but told @romani some things I dislike about the new folders.
We now have styleguide listed twice in the URL, and I don't think google.github.io is necessary or pretty.

@Vampire
Copy link
Contributor Author

Vampire commented May 6, 2019

Well, my intention was to keep it like the website copier left it for easy updating and diffing in the future.
If you guys really have a problem with the paths, we can change that of course.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good work! Users will appreciate this update a lot. Thanks a lot!

attention to #6620 (comment)

few minors:

src/xdocs/google_style.xml Show resolved Hide resolved
Google Java Style</a>(<a
href="styleguides/google-java-style-20180523/google.github.io/styleguide/javaguide.html"
>cached page</a>),
version of 23 May 2018, current as of 30 March 2019
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why we need "30 March", what is reason to have it ?
Not sure why user need to know date of export. We use date as some sort of anchor to commit in googe style repo.
it is probably better to place SHA1 of commit/link-to-commit to be even more exact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I as user would be interested in this information tbh.
It is more visible with the sun style where we have "version of 20 April 1999, current as of 30 March 2019".
It shows the user when was last checked that the cached and reported-on version is current.
If I only read "version of 20 April 1999" or "version of 23 May 2018", I ask myself "is this also compatible with the latest version, were there any changes?".
If I see that it was last checked on "30 March 2019", I'm pretty confident the report is against a pretty recent version of the styleguide.
(Updated to 07 May 2019 now, as I just rechecked it is still recent)

<a
href="config_whitespace.html#MethodParamPad">MethodParamPad</a>
<br />
<br />
<img src="images/ok_green.png" alt="" />
<img src="images/ok_blue.png" alt="" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets keep all non-NoWhitespaceBefore as green because "Existing Check covers all requirements from Google.".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I interpreted it as "the combination of these checks covers all requirements.
Because actually none of the single checks "covers all requirements".
That's why I switched them all to blue and maybe there should actually be only one icon per styleguide rule, not one icon per checkstyle rule.
For now I switched the others back to green as requested.

src/xdocs/style_configs.xml Show resolved Hide resolved
@romani romani merged commit 9a2f293 into checkstyle:master May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cached Google Style is slightly outdated and not like original
3 participants