robot
robot copied to clipboard
Robot header
Resolves #1152
- [x]
docs/have been added/updated - [x] tests have been added/updated
- [x]
mvn verifysays all tests pass - [x]
mvn sitesays all JavaDocs correct - [x]
CHANGELOG.mdhas been updated
Adds option to input template strings via an external file as described in Issue #1152
My first thought when I see the details of this implementation is that the row numbers in our error messages will be off by one, which will be confusing.
@pk-mitre I apologize for taking so long to review this PR. This is a good feature, and we really do appreciate your contribution! The problem is simply that nobody on the core ROBOT team has dedicated hours for this work.
I made a few specific suggestions. If you have time to reply or make those changes, it would be appreciated. If you don't have time, let me know and I'll try to find time to finish the PR myself.
The only major question I have at this point is: What to do if the template and the ext-template do not match? Eventually someone will add new columns to one and forget to add them to the other, then file a bug report here.
I think that that best approach might be to check that the first row of the template and the ext-template are identical. What do you think?
Thanks again!
James: I have addressed the code comments and updated the pull request code. I will follow up on the mismatch issue between the template and the ext-template Thanks
@pk-mitre Thank you for your continued work on this PR. I'm sorry that our schedules never seem to be in sync.
Do you still plan to follow up on the case of a mismatch between the template and the ext-template? Please let me know either way. Thanks again!
@jamesaoverton Yes, sorry I was working on a few other things. I will be following up on this mismatch issue and check things in. Thanks !
@jamesaoverton Am working through any potential additional error handling. With reference to your suggestion about checking the first row of the template file against the external template,I assume you mean the column count ? Since the first row in the template file is the header information, so it wouldn't be identical with the externalized template strings even in a correct case. For example:
Template file:
CURIE Label Parent Comment obo:0000001 animal Any animal in the world. obo:0000002 canine animal A member of the genus Canis. obo:0000003 feline animal A member of the genus Felis.
external template file:
ID LABEL SC % A rdfs:comment
@pk-mitre You're right, my request was confused.
I'm worried about users changing the column structure of the template without changing the structure of the external template, or the other way around. I'm also worried that it's hard to tell which ROBOT template strings apply to which columns in the external template file.
So now I suggest that the external template file has two rows: the first for header strings and the second for ROBOT template strings. ROBOT should then check that the first row of the two files is the same: the headers in the template file match the headers in the external template file. If the first rows of the two files are not exactly the same, ROBOT should quit with an error message.
Template file:
CURIE Label Parent Comment obo:0000001 animal Any animal in the world. obo:0000002 canine animal A member of the genus Canis. obo:0000003 feline animal A member of the genus Felis.
external template file:
CURIE Label Parent Comment ID LABEL SC % A rdfs:comment
@jamesaoverton I have made the changes for the external template header error checking and also the file name changes to make it clearer as indicated in your comment on the naming.
Thanks @pk-mitre! I skimmed the changes and they look good. I have a big deadline tomorrow, but I'll plan to finish reviewing on Friday.
I'm going to merge this and make a new PR with a few changes.
Sorry that this one took so long. Thanks for hanging in there and getting it done!
@jamesaoverton Thanks for the merge, appreciate the work and the time.
@jamesaoverton Thanks ! Glad it all merged in smoothly.
@jamesaoverton This one line has to be updated to take into account the changed flag name:
if (options.get("external-template") != null) { template.setRowNum(1); }
File: TemplateOperation.java
You're right. I fixed that in dc04a3c97b89c1306245479740cab50d615342f0. I really wish we had a test to catch that, but I ran into a bunch of problems verifying this, and used up more time than I could afford.
@jamesaoverton Thanks for all your help on this ticket !