Skip to content

Issue #3796: No unnecessary text shall be present in Javadoc ASTs#4462

Merged
romani merged 1 commit intocheckstyle:masterfrom
therootusr:issue_3796
Jun 21, 2017
Merged

Issue #3796: No unnecessary text shall be present in Javadoc ASTs#4462
romani merged 1 commit intocheckstyle:masterfrom
therootusr:issue_3796

Conversation

@therootusr
Copy link
Copy Markdown

Issue #3796: No unnecessary text shall be present in Javadoc ASTs

@therootusr
Copy link
Copy Markdown
Author

@rnveach @romani @Vladlis Please have a look. Thanks.

@therootusr
Copy link
Copy Markdown
Author

therootusr commented Jun 16, 2017

I was going to make DetailNodeTreeStringPrinterTest extend BaseCheckTestSupport for this one but I didn't. I feel it should be handled in #4381 since the refactoring of the Javadoc tests in that issue may even demand that we change their classes. Please check this comment

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jun 16, 2017

Codecov Report

Merging #4462 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4462   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         285     285           
  Lines       15316   15323    +7     
  Branches     3484    3487    +3     
======================================
+ Hits        15316   15323    +7
Impacted Files Coverage Δ
...checkstyle/checks/javadoc/SummaryJavadocCheck.java 100% <100%> (ø) ⬆️
...rawl/tools/checkstyle/JavadocDetailNodeParser.java 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d231ed1...2bfa68b. Read the comment docs.

@rnveach
Copy link
Copy Markdown
Contributor

rnveach commented Jun 16, 2017

I feel it should be handled in #4381

I thought we were going to make that a priority and do it next since we keep bringing it up and wanting it.

Copy link
Copy Markdown
Contributor

@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.

We need to update all xdocs with changes to printed tree.
Example: http://checkstyle.sourceforge.net/writingjavadocchecks.html#Tool_to_print_Javadoc_tree_structure

We will need both, ANTLR regression and SummaryJavadocCheck regression.
We expect no changes in SummaryJavadocCheck.
We should probably also see that the changes didn't hurt the GUI application any.

@therootusr
Copy link
Copy Markdown
Author

therootusr commented Jun 17, 2017

@rnveach look at this. 4381 has this as a precursor. We can't do 4381 without this or else we would have to again change all the trees we create in 4381. 4381 is indeed a priority.

Almost every node with children would be a change in ANTLR regression. Perhaps it would suffice to run ANTLR regression on a couple of projects with extensive javadoc ?

@therootusr
Copy link
Copy Markdown
Author

We need to update all xdocs with changes to printed tree.

done.

@rnveach
Copy link
Copy Markdown
Contributor

rnveach commented Jun 17, 2017

We can't do 4381 without this or else we would have to again change all the trees we create in 4381

Seeing more changes in test wouldn't be a bad thing. Before the xdoc change, there was not a lot of differences in the tests.

@therootusr
Copy link
Copy Markdown
Author

@rnveach
We will have to redo all the nodes with children for all the input files we create in 4381 if we do 4381 before this ? I too feel it is better to have this one done before 4381. Do you want me to do 4381 before this ?

@rnveach
Copy link
Copy Markdown
Contributor

rnveach commented Jun 17, 2017

you want me to do 4381 before this ?

We will finish this.

https://ci.appveyor.com/project/Checkstyle/checkstyle/build/7927/job/v9uu51qsij0y9mcm
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.6:site (default-site) on project checkstyle: failed to get report for org.apache.maven.plugins:maven-project-info-reports-plugin: Unable to load the mojo 'dependencies' (or one of its required components) from the plugin 'org.apache.maven.plugins:maven-project-info-reports-plugin:2.9': com.google.inject.ProvisionException: Unable to provision, see the following errors:
[ERROR]
[ERROR] 1) No implementation for org.apache.maven.shared.jar.classes.JarClassesAnalysis was bound.
[ERROR] while locating org.apache.maven.report.projectinfo.DependenciesReport
[ERROR] at ClassRealm[plugin>org.apache.maven.plugins:maven-project-info-reports-plugin:2.9, parent: ClassRealm[plugin>org.apache.maven.plugins:maven-site-plugin:3.6, parent: sun.misc.Launcher$AppClassLoader@55f96302]] (via modules: org.eclipse.sisu.wire.WireModule -> org.eclipse.sisu.plexus.PlexusBindingModule)
[ERROR] while locating org.apache.maven.plugin.Mojo annotated with @com.google.inject.name.Named(value=org.apache.maven.plugins:maven-project-info-reports-plugin:2.9:dependencies)

This is a weird error I haven't seen before.
I will try to restart.

@therootusr
Copy link
Copy Markdown
Author

@rnveach yeah thanks. mvn -e -Pno-validations site ended with BUILD SUCCESS for me

@therootusr
Copy link
Copy Markdown
Author

therootusr commented Jun 17, 2017

summary javadoc diff report

antlr diff //taken xrefs out. The size went beyond 800mb with xrefs most of which was taken up by guava's

Let me know if more diff reports are needed

@rnveach rnveach requested a review from romani June 17, 2017 17:12
@rnveach
Copy link
Copy Markdown
Contributor

rnveach commented Jun 19, 2017

@romani This PR blocks all other @PS-SP PRs because they rely code for #4381. @PS-SP and mentor want this first before they start that issue.

@romani romani merged commit 3744f4c into checkstyle:master Jun 21, 2017
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.

5 participants