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?
Summary of discussion with @pzygielo on #1960:
Report::size/getNumViolationsneeds a comment because it's the size of theiteratorReport::isEmptydoesn't necessarily mean that there are some violations, which is counter-intuitive (it considers violations and processing errors).So maybe
isEmptyis badly named. However it looks like the issue is a bit bigger. TheisEmptymethod is useless on its own, as it doesn't say anything about either the size ofiterator()or oferrors(), 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 toisEmptyand will need another check (either hasErrors, or hasNext on the iterator, or a zerosize()check).So probably,
isEmptyshouldn't even exist. But maybe the problem is even bigger.The
Reportclass appears to try being three lists at a time, as evidenced by the following methods:boolean isEmpty()boolean hasErrors()boolean hasConfigErrors()hasViolations)Iterator<RuleViolation> iterator()Iterator<ProcessingError> errors()Iterator<ConfigurationError> configErrors()int size()numErrorsornumConfigErrors)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?