[#1017] updating missing_label query to support empty labels #1018
Conversation
There was a problem hiding this comment.
@psiotwo looks great! One (apart from the individual comment):
Is every single change you make here absolutely necessary for the PR? This is a very important criterion for us now, because of the cost of accidentally introducing non obvious bugs.
Thank you for your contribution!
docs/report_queries/missing_label.md
Outdated
| **OBO Foundry Principle:** [12 - Naming Conventions](http://www.obofoundry.org/principles/fp-012-naming-conventions.html) | ||
|
|
||
| **Solution:** Add a label. | ||
| # # Missing Label |
There was a problem hiding this comment.
Introduction of redundant # for the MD file!
There was a problem hiding this comment.
Yes. Better to use the https://github.com/ontodev/robot/blob/master/docs/Makefile to generate the doc from the query file.
There was a problem hiding this comment.
Hm, actually deleting docs/report_queries/missing_label.md and (naively) running make inside docs does generate these # in the MD file ... if I didn't miss any config, the only thing that jumps into my mind now is that some sed regex might behave differently on various platforms (I am using M1 Mac) ... but not sure, will take a look later.
There was a problem hiding this comment.
I have just manually removed the #s at present.
| for (String qPath : queryFilePaths) { | ||
| String ruleName = qPath.substring(qPath.lastIndexOf("/")).split(".")[0]; | ||
| for (final Path qPath : queryFilePaths) { | ||
| final String ruleName = qPath.getFileName().toString().split("[.]")[0]; |
There was a problem hiding this comment.
These kinds of changes are very hard to review and we prefer in the interest of resources to only do them if we have an issue that cleary explains what is wrong, with agreement all around it absolutely needs changing and then changing it. Are they absolutely essential to this PR?
There was a problem hiding this comment.
Thanks @matentzn for asking to keep the PR focused, and sorry if I’m being inconsistent, but this path handling looks like a nice improvement.
There was a problem hiding this comment.
@matentzn you are absolutely right. Actually, I originally wanted to "hang" the test on the existing API of ReportOperation - these changes were needed to make the profile file processing actually working from tests.
But this would have meant going through profile.txt which would be an extra overhead so I reconsidered and used the getViolations method to keep it focused. This bit of code belonged to the original story and I accidentally committed it.
Now I reverted it back. I also don't like these types of extra changes - doing unnecessary changes when there are almost no tests around is dangerous.
Resolves [#1017]
docs/have been added/updatedmvn verifysays all tests passmvn sitesays all JavaDocs correctCHANGELOG.mdhas been updatedAdded a condition checking for empty labels (also across languages) "" into the
missing_label.rq.The test code added is more general than needed to allow adding other query tests easily.
I did some ad-hoc performance tests just on my Mac on DOID, EFO and HP - all showing up to 10% performance degradation with the new query.
@matentzn if you have a more representative benchmark to push it through, would be great!