Skip to content

Issue #13097: Split annotation xdocs into separate pages#13103

Merged
romani merged 4 commits intocheckstyle:masterfrom
stoyanK7:issue/13097-annotations
Jun 16, 2023
Merged

Issue #13097: Split annotation xdocs into separate pages#13103
romani merged 4 commits intocheckstyle:masterfrom
stoyanK7:issue/13097-annotations

Conversation

@stoyanK7
Copy link
Contributor

Part of #13097

@stoyanK7
Copy link
Contributor Author

Checks will fail in a few minutes, there are a couple of tests that would need attention(rewriting, deleting, I don't know yet)

@@ -1,1571 +1,94 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would personally rename this file to xdocs/checks/annotation/index.html. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member

Choose a reason for hiding this comment

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

@rnveach , please look also.

@stoyanK7
Copy link
Contributor Author

Github, generate site

@github-actions
Copy link
Contributor

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks/annotation/annotationlocation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks/annotation/annotationonsameline.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks/annotation/annotationusestyle.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks/annotation/missingdeprecated.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks/annotation/missingoverride.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks/annotation/packageannotation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks/annotation/suppresswarnings.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/checks/annotation/suppresswarningsholder.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/config_annotation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/config_filters.html#SuppressWarningsFilter_Description

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/google_style.html#Google_Coverage_table

@stoyanK7
Copy link
Contributor Author

Failures to be addressed: https://dev.azure.com/romanivanovjr/romanivanovjr/_build/results?buildId=14476&view=logs&j=c902ebb4-c9f8-5f09-4e17-ff78fbbc842e&t=9ca98c81-ff64-58f0-9d03-a23ac1c4a111&l=1023

[ERROR]   AllChecksTest.testAllCheckstyleModulesHaveXdocDocumentation:495->lambda$testAllCheckstyleModulesHaveXdocDocumentation$4:499 Module AnnotationUseStyle does not have xdoc documentation.
[ERROR]   XdocsPagesTest.testAllChecksPresentOnAvailableChecksPage:232->lambda$testAllChecksPresentOnAvailableChecksPage$1:236 AnnotationUseStyle is not correctly listed on Available Checks page - add it to src/xdocs/checks.xml
[ERROR]   XdocsPagesTest.testAllStyleRules:1531->validateStyleModules:1725 google_style.xml rule '4.8.5 Annotations' has too many configs
expected to be false
[ERROR] Errors: 
[ERROR]   XdocsJavaDocsTest.testAllCheckSectionJavaDocs:166->examineCheckSection:179 Checkstyle config_annotation.xml couldn't find class: Annotation Checks
[ERROR]   XdocsPagesTest.testAllCheckSections:501->validateCheckSection:546 Checkstyle config_annotation.xml couldn't find class: Annotation Checks
[INFO] 
[ERROR] Tests run: 4059, Failures: 3, Errors: 2, Skipped: 2

@romani
Copy link
Member

romani commented May 30, 2023

@stoyanK7 , can you apply your fix for tables rows to be vertical in check group index pages?
to let me play with it from phone.

@stoyanK7
Copy link
Contributor Author

@romani The table is available at https://stoyank7.github.io/checks.html. Or do you want me to add the fix in this PR too?

@romani
Copy link
Member

romani commented May 31, 2023

Or do you want me to add the fix in this PR too?

Add it to this PR, it can be as separate commit if that is convenient for you to keep it separate.

@nrmancuso nrmancuso self-requested a review May 31, 2023 02:05
@stoyanK7
Copy link
Contributor Author

There are 2 remaining failing tests. Both of them are looking for a section for each module but we remove sections and use table now.
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a202661_2023185812/config_annotation.html#
Screenshot from 2023-05-31 13-31-40

@romani @nrmancuso Before I extend these tests, is this what we are going with? A table like the above?

[ERROR]   XdocsJavaDocsTest.testAllCheckSectionJavaDocs:166->examineCheckSection:179 Checkstyle index.xml couldn't find class: Annotation Checks
[ERROR]   XdocsPagesTest.testAllCheckSections:501->validateCheckSection:546 Checkstyle index.xml couldn't find class: Annotation Checks

@stoyanK7
Copy link
Contributor Author

Add it to this PR, it can be as separate commit if that is convenient for you to keep it separate.

Done.

@romani
Copy link
Member

romani commented May 31, 2023

A table like the above?

I think test is just strict enforcement of doc to be present. Back in several years, web site was very very very inconsistent. So such hack in Junit helped put situation under control.

Now layout are changing, we do not expect new Checks in near future, so we disable this check or some part of it and create issue to come back to it at the end , when we will have already final layout.

It was part of project to setup best practices ( wiki page) and enforce some of them. Please create create two issues to deal with it later at summer.

@stoyanK7
Copy link
Contributor Author

It was part of project to setup best practices ( wiki page) and enforce some of them. Please create create two issues to deal with it later at summer.

Done at #13118 and #13117

@romani
Copy link
Member

romani commented Jun 1, 2023

Not enough.
Please create issue for wiki page creation that we can point to new contributor and explain less on what to do.

@stoyanK7
Copy link
Contributor Author

stoyanK7 commented Jun 1, 2023

Please create issue for wiki page creation that we can point to new contributor and explain less on what to do.

Done. As #13117 gets approved and merged, I will rebase and push the latest changes and change to "Ready for review."

@nrmancuso
Copy link
Contributor

@stoyanK7 please rebase and prepare for final reivew

@stoyanK7 stoyanK7 marked this pull request as ready for review June 1, 2023 17:33
@stoyanK7
Copy link
Contributor Author

stoyanK7 commented Jun 1, 2023

Github, generate site

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/annotationlocation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/annotationonsameline.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/annotationusestyle.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/index.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/missingdeprecated.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/missingoverride.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/packageannotation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/suppresswarnings.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/checks/annotation/suppresswarningsholder.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/config_annotation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/config_filters.html#SuppressWarningsFilter_Description

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4607166_2023174735/google_style.html#Google_Coverage_table

Copy link
Contributor

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

First pass:

@stoyanK7
Copy link
Contributor Author

stoyanK7 commented Jun 1, 2023

Github, generate site

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/annotationlocation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/annotationonsameline.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/annotationusestyle.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/index.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/missingdeprecated.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/missingoverride.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/packageannotation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/suppresswarnings.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/checks/annotation/suppresswarningsholder.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/config_annotation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/config_filters.html#SuppressWarningsFilter_Description

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/d1819a5_2023221314/google_style.html#Google_Coverage_table

@romani
Copy link
Member

romani commented Jun 10, 2023

I like layout.
There only pop point to improve is to make name of property to be italic to look different than description.

Screenshot_20230610-135311

But we can definitely this in separate PR. If you agree, please create issue to deal with this later.

Created: #13195

@romani
Copy link
Member

romani commented Jun 10, 2023

annotationlocation.html#Package I feel this section is a little redundant now

Let's keep it for now, and we will rethink on it later on.
I don't remember reason of it but it might good for something to show fully qualified name of Check.

Created: #13194

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.

Ok to merge.
I hope we can fix all issues separately before end of month, to not damage user experience

@rnveach
Copy link
Contributor

rnveach commented Jun 11, 2023

There only pop point to improve is to make name of property to be italic to look different than description.

This did not happen in the production page, what caused this change in this PR? Is this the same issue as the missing JS?

We can have regression here for for month or two.
hope we can fix all issues separately before end of month, to not damage user experience

Conflicting statements.

@rnveach
Copy link
Contributor

rnveach commented Jun 11, 2023

However, I don't think we have to bloat the page with information just because space needs to be filled up. Documentation should be straight to the point, no?

So what is the point of a page that just contains links to other pages? We already had https://checkstyle.org/checks.html and it made sense since it was our master list. I don't see users saying "I wonder which of these checks are only connected to Annotations".

Also, are we keeping the master list as well as adding these sub-lists? So we are going to have to verify all of them are correct and always up to date.

Also, I don't see any tests being written here. Is there another thing being delayed and is there an issue to do this?

almost nobody need to know how to write Check or Filter, 99% of users just need information on how to use/configure module.

It is about putting things of a similar subject together. I am not talking about moving the entire documentation to the new page. Just add a simple blurb and link. We are reworking documentation already, I see it makes sense to rework some of the other pages that connect to this.

we need some sort of table/list to show all modules of group

Why?

@rnveach
Copy link
Contributor

rnveach commented Jun 11, 2023

Still seeing this as my points so far:

  • a short blurb about what "Annotations" is
  • these sub-pages should have a link somewhere on them to take them back to the package page and on the package page back to the checks.html page
  • tests
  • mobile properties table

=======

From looking at the new site generation,

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/8cea5c0_2023162451/checks/annotation/annotationlocation.html#AnnotationLocation
Left nav links should be actual words. Spaces are missing.

Copyright © 2001–2023..

This is not related to your changes, but we should remove the double period in the copyright on the bottom right.

@romani
Copy link
Member

romani commented Jun 11, 2023

This did not happen in the production page, what caused this change in this PR?

It is innovation of this PR, that is visible only from mobile phone. I like it , but it needs minor improvement.

Conflicting statements.

I am ok with regression unit end of month for sure and even until end of year.

So what is the point of a page that just contains links to other pages?

For now just limitation on our web site structure, and xdoc model. I created context section a long time ago to show all in group, it was very useful. Especially for javadoc group.
I do not want to discuss regrouping in this PR. I just try to do step by step.

Also, I don't see any tests being written here. Is there another thing being delayed and is there an issue to do this?

There is no automation or any validation in this PR, as we try all agree on how it will look like, so automation to enforce can be done later on in separate issue and PR. We already have issue to think validation.

Just add a simple blurb and link. We are reworking documentation already, I see it makes sense to rework some of the other pages that connect to this.

We can discuss it separately. I do not know what to put there. I do not see it at all, but may be you can show us what you mean. I still believe that better to keep usage doc separately from development doc. This my belief, blocks my mind to accept your idea, but may be I need small PR to show how you see it.

Why?

I don't know, this how my brain works, I use it/content section all the time. We can look at history, it might be some feedback from user guide me to spend time to make it. I remember it was not simple to do.

short blurb about what "Annotations" is

Please provide text to put. I do not know what is good to put here.

these sub-pages should have a link somewhere on them to take them back to the package page and on the package page back to the

Left panel shows location, as in all other pages . Please do image to point where you want more links , and share why left panel is not working for you.

This is not related to your changes, but we should remove the double period in the copyright on the bottom right.

This issue even before this PR. I tried to investigate why it happens long time ago, but it is from site plugin , there is no content that we can change, or I do not know how to do this.

Spaces are missing.

I don't understand. Please point more explicitly.
Name of Check should be without spaces in name.

tests
mobile properties table

We have separate issues for this. Let's focus on big picture of this update. After merge of this PR we can start fixing all that issues in parallel, to ease development and review.

@romani
Copy link
Member

romani commented Jun 12, 2023

@rnveach , please help us to unblock this PR and let us be on same page, let us know if something is not covered by extra issue

@stoyanK7
Copy link
Contributor Author

Github, generate website

@github-actions
Copy link
Contributor

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/annotationlocation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/annotationonsameline.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/annotationusestyle.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/index.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/missingdeprecated.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/missingoverride.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/packageannotation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/suppresswarnings.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/checks/annotation/suppresswarningsholder.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/config_annotation.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/config_filters.html#SuppressWarningsFilter_Description

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/google_style.html#Google_Coverage_table

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/releasenotes_old_1-0_5-9.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/906dcc8_2023190416/releasenotes_old_6-0_7-8.html#

@stoyanK7
Copy link
Contributor Author

@rnveach @romani Anchors and To Top are fixed.

@romani
Copy link
Member

romani commented Jun 15, 2023

all works good.
Thanks a lot for fixes.
@rnveach , please finalize review, if something can be done separately, please approve PR and I will create such issues.

@rnveach
Copy link
Contributor

rnveach commented Jun 15, 2023

minor: add $relativePath variable to resolve pathing issues

This should be connected to the issue we opened. We should also add more details to it about $relativePath and how it came from the default site for doxia. No one will remember this and honestly this could break in the next upgrade.

====

I am not really against this, I just think more could be done for it either here or in new issues. If it is not done here, then I would like to see new issues added.

@rnveach rnveach assigned romani and unassigned rnveach Jun 15, 2023
@stoyanK7
Copy link
Contributor Author

This should be connected to the issue we opened.

Done.

We should also add more details to it about $relativePath and how it came from the default site for doxia. No one will remember this and honestly this could break in the next upgrade.

I added some brief explanation on how it came to be at #13190 (comment)

@romani
Copy link
Member

romani commented Jun 15, 2023

@rnveach , thanks a lot, we will proceed with merge after CI is green. I think we opened all issues already.

@stoyanK7 , can we make CI green ?

@stoyanK7
Copy link
Contributor Author

@romani All these errors are external links. Linkcheck is failing in other PRs that are not related to docs too.

@stoyanK7
Copy link
Contributor Author

As stated in #13214 (comment), Stackoverflow is having issues, that's why CI is red.

@romani romani merged commit fb97cb4 into checkstyle:master Jun 16, 2023
@romani
Copy link
Member

romani commented Jun 16, 2023

Ok, let's move forward.

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.

4 participants