javadoc-coverage icon indicating copy to clipboard operation
javadoc-coverage copied to clipboard

Add possibility to check only public coverage

Open stieglma opened this issue 6 years ago • 7 comments

stieglma avatar Jan 18 '18 13:01 stieglma

what do you want to change? The second pull request I issued relies already on some parts of these changes here. (This is also the version I currently use, I checked the numbers and they all seem to be correct there)

stieglma avatar Jan 25 '18 21:01 stieglma

if it's not too much I'd prefer when you put comments to the locations you want to have changed (and also the way you want to have them change) I'll do it until sometime saturday then

stieglma avatar Jan 25 '18 21:01 stieglma

I haven't reviewed all the changes yet. Up to now, I performed some small changes I describe below, which are related to format and documentation only:

  1. Small documentation fixes and reorganization to conform to the general format used in the project. For instance, the documentation for a param should be placed after its name, not in the next line.
  2. Optimized imports: when there are more than 3 imports from the same package, an import package.* is used to avoid a long list of imports that just makes the code less clean. This approach has no impact on compiled class size, since Java compiler includes just the used classes into the bytecode. This way, the use of * is being preferred and it's automatically performed by IntelliJ.
  3. Reformated some long lambda expressions to break line after each method call. This is a good practice to make lambdas clearer.

There is a major issue which will require several structural changes. I wonder if you could fix it as I describe below (after merging the changes I've already done).

  1. You included the computeOnlyForPublicModifier attribute into the CoverageDoclet class. This attribute is being passed around several classes. Even where there is access to a CoverageDoclet object, such as inside the AbstractDataExporter class, the attribute is passed as a parameter to other classes. That is the case at line 61 of the AbstractDataExporter. The JavaDocsStats requires a RootDoc object which is got from the doclet. In the same way, the value for the computeOnlyForPublic parameter is being got from doclet. That suggests you could just pass the doclet as a single parameter and get what you want from it inside the JavaDocsStats. The same applies for all DocStats classes.
  2. Multiple methods from classes such as ClassDocStats received a new computeOnlyForPublic. When you pass the same parameter around the methods of a class, that suggests you should create an attribute and remove such parameter from the methods. For such classes, applying the approach proposed above fix this issue.

There may be other small issues. Tell me the way you prefer to approach our changes.

manoelcampos avatar Jan 26 '18 12:01 manoelcampos

I added some rework of the docmentation and formatting stuff. As I work with eclipse it may not comply 100% to the settings you have in IntelliJ. As this is a common problem I propose to use the google-java-formatter instead. It has quite reasonable formatting defaults and will produce the same formatting for all IDEs.

To the second point regarding passing the CoverageDoclet as a parameter around: This is something I wouldn't do as it has many more methods that should not ever be called inside all the javadoc computation classes. I was writing it this way, in order not to change the whole code at this point. But the way to go would be to create a separate configuration container class (lets call it Configuration ;) ) which contains the rootDoc and the options which are set or not. This way it would also become much easier to add new options (similar to passing the doclet, but this has just some other purpose and shouldn't be used for that case)

stieglma avatar Jan 27 '18 11:01 stieglma

Sorry for the late reply. I think that creating a Configuration class is an excellent option. I appreciate if you could follow this path. However, since the Doclet is already being passed around, a new Configuration configuration attribute could be defined inside the Doclet class. This way, the configuration could be got directly from the Doclet, not requiring to add an extra parameter to existing methods.

I'm going to include some comments on the code you pushed.

manoelcampos avatar Feb 04 '18 23:02 manoelcampos

Configuration handling rework is now done. Took a while, but I didn't have much time the last weeks

stieglma avatar Feb 10 '18 13:02 stieglma

I also added lombok as a dependency. I greatly reduces boilerplate code that would have to be written otherwise. If you don't like to have this dependency we should at least think about using guava for less verbose precondition checks.

stieglma avatar Feb 10 '18 13:02 stieglma