Skip to content

[core] Simplify Report API #1962

@oowekyala

Description

@oowekyala

Summary of discussion with @pzygielo on #1960:

  • Report::size/getNumViolations needs a comment because it's the size of the iterator
  • Report::isEmpty doesn't necessarily mean that there are some violations, which is counter-intuitive (it considers violations and processing errors).

So maybe isEmpty is badly named. However it looks like the issue is a bit bigger. The isEmpty method is useless on its own, as it doesn't say anything about either the size of iterator() or of errors(), since the results are merged. It also doesn't take config errors into account which undermines its only usefulness (maybe checking that there is nothing to report, but with this implementation there can still be config errors). So you can't derive anything useful from a method call to isEmpty and will need another check (either hasErrors, or hasNext on the iterator, or a zero size() check).

So probably, isEmpty shouldn't even exist. But maybe the problem is even bigger.
The Report class appears to try being three lists at a time, as evidenced by the following methods:

  • boolean isEmpty()
  • boolean hasErrors()
  • boolean hasConfigErrors()
  • (notice no hasViolations)
  • Iterator<RuleViolation> iterator()
  • Iterator<ProcessingError> errors()
  • Iterator<ConfigurationError> configErrors()
  • int size()
  • (notice no numErrors or numConfigErrors)

I shouldn't be mentioning the ReportTree and Metric methods, because they're supposed to be deprecated, but they aren't for now so let's just make a list to deprecate them later:

  • treeIterator(), treeIsEmpty(), treeSize()
  • metrics(), hasMetrics()

So Report attempts to be 5 collections at a time and does a meh job of it, because the API is assymmetric and very rudimentary.

I think making Report expose those collections independently would be a big upgrade to that API. We could replace all the above-mentioned methods by just

  • List<RuleViolation> getViolations()
  • List<ProcessingError> getProcessingErrors()
  • List<ConfigurationError> getConfigErrors()

This would reduce greatly the API surface we have to maintain while improving the API available to users. Besides, since Report is implemented with Lists internally, it would be trivial to implement.

Making this change would supersede #1960

Should we do that?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions