robot icon indicating copy to clipboard operation
robot copied to clipboard

Robot header

Open pk-mitre opened this issue 2 years ago • 7 comments

Resolves #1152

  • [x] docs/ have been added/updated
  • [x] tests have been added/updated
  • [x] mvn verify says all tests pass
  • [x] mvn site says all JavaDocs correct
  • [x] CHANGELOG.md has been updated

Adds option to input template strings via an external file as described in Issue #1152

pk-mitre avatar Sep 29 '23 21:09 pk-mitre

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.

jamesaoverton avatar Oct 02 '23 13:10 jamesaoverton

@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!

jamesaoverton avatar Nov 29 '23 15:11 jamesaoverton

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 avatar Jan 09 '24 20:01 pk-mitre

@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 avatar Feb 07 '24 19:02 jamesaoverton

@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 !

pk-mitre avatar Feb 07 '24 21:02 pk-mitre

@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 avatar Mar 06 '24 20:03 pk-mitre

@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 avatar Mar 07 '24 15:03 jamesaoverton

@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.

pk-mitre avatar Mar 27 '24 18:03 pk-mitre

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.

jamesaoverton avatar Mar 27 '24 18:03 jamesaoverton

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 avatar Mar 29 '24 20:03 jamesaoverton

@jamesaoverton Thanks for the merge, appreciate the work and the time.

dlutz2 avatar Mar 30 '24 07:03 dlutz2

@jamesaoverton Thanks ! Glad it all merged in smoothly.

pk-mitre avatar Mar 31 '24 23:03 pk-mitre

@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

pk-mitre avatar Apr 17 '24 16:04 pk-mitre

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 avatar Apr 17 '24 17:04 jamesaoverton

@jamesaoverton Thanks for all your help on this ticket !

pk-mitre avatar Apr 18 '24 14:04 pk-mitre