Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update repair command to allow migration of annotation values #510

Merged
merged 5 commits into from
Aug 21, 2019

Conversation

balhoff
Copy link
Contributor

@balhoff balhoff commented Jun 14, 2019

See #492.

Syntax: robot repair --input xref-need-of-repair.obo --annotation-property oboInOwl:hasDbXref --output results/xref-repaired.obo

… which to migrate values for deprecated terms. See ontodev#492.
@balhoff balhoff requested a review from cmungall June 14, 2019 18:36
@balhoff balhoff changed the title Update repair command migrate annotation values Update repair command to allow migration of annotation values Jun 14, 2019
@balhoff
Copy link
Contributor Author

balhoff commented Jun 15, 2019

This PR addresses @cmungall's request:

add an option to migrate selected (or ALL) annotation assertions

However I didn't implement the ALL option. I don't think we would want to accidentally migrate owl:deprecated or rdfs:label. I suppose ALL could exclude just those two, but I think it is less magical to list the desired properties. But I am open to changing course.

@jamesaoverton
Copy link
Member

@balhoff Thanks for including automated tests. ROBOT will run the integration tests during mvn verify with the example code from repair.md and check the example files, so it should not be necessary to have the same files tested as a unit test in RepairOperationTest.java.

When just comparing inputs to outputs, I prefer just the integration tests. When comparing different method calls, just the unit tests are better. If that's not clear, we can add better documentation to CONTRIBUTING.md.

@jamesaoverton
Copy link
Member

@rctauber suggests that using an option map and resolving the annotation properties inside the operation will make life easier going forward, as more options are added to the command.

Copy link
Contributor

@cmungall cmungall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! This covers the case when the subject is obsoleted - for the case when the object is obsoleted, shall we do a separate PR or roll into this one?

@balhoff
Copy link
Contributor Author

balhoff commented Jun 27, 2019

@jamesaoverton @rctauber can you say more about what you mean by "option map"? Is there another command that works this way which I can look at?

Oh—I think you mean passing a Map structure to the Operation.

@jamesaoverton
Copy link
Member

Yes, I think Chris pioneered the use of Map<String,String> structures to pass options from a Command to an Operation. One benefit is avoiding the need to update the method signatures every time. Here's an example: https://github.com/ontodev/robot/blob/master/robot-core/src/main/java/org/obolibrary/robot/ReasonOperation.java#L43

@balhoff
Copy link
Contributor Author

balhoff commented Jul 9, 2019

@jamesaoverton @rctauber the method signatures in RepairOperation do already have a parameter for an option map. I looked at accepting the list of annotation properties this way, but it bothers me to mix command-line parsing into the operation (this is done in my PR in the command class using CommandLineHelper). I like the distinction in the ROBOT readme: "They should not contain IO or CLI code." So if you think method signatures have proliferated too much (I left the previous versions to not break binary compatibility) I can adapt, but I'm inclined to leave it this way.

Copy link
Member

@jamesaoverton jamesaoverton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like properties is never used.

@jamesaoverton
Copy link
Member

@balhoff Ok, I'm fine with that. My only remaining comment is that the properties set is constructed but no used in RepairCommand.java.

@balhoff
Copy link
Contributor Author

balhoff commented Aug 21, 2019

@balhoff Ok, I'm fine with that. My only remaining comment is that the properties set is constructed but no used in RepairCommand.java.

Thanks for catching this! 😬 I fixed that, and also another issue I discovered with mergeAxiomAnnotations not being read correctly. Sorry for the long delay; this should be ready to review/merge.

@jamesaoverton jamesaoverton merged commit 3f25cc4 into ontodev:master Aug 21, 2019
@jamesaoverton
Copy link
Member

Thanks @balhoff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants