Add Op Parameter Nullability, Java Field Op Parameter Types to YAML#216
Merged
Add Op Parameter Nullability, Java Field Op Parameter Types to YAML#216
Conversation
1965737 to
deacfe7
Compare
This commit makes many different changes/bugfixes, all tied to enabling richer YAML output by the scijava-ops-indexer, and to enabling richer usage within the scijava-ops-engine.
deacfe7 to
d6a8539
Compare
hinerm
requested changes
May 15, 2024
Member
hinerm
left a comment
There was a problem hiding this comment.
Thanks @gselzer. Given that things seem to still be working with these changes I gave it a fairly cursory look. Aside from these minor points, consider re-testing the paper use-cases just to make sure everything is still good there.
And we don't need to resolve the scijava-ext-parser at this time given that its output YAML is still working (as I tested the OpenCV use case)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR, designed to close #200, by adding a
nullablefield in the YAML to each parameter of each Op. Currently, the indexer fills this field by:@Nullableannotation on any method parameters (suitable for Ops written as Java classes and methods), recording that parameter as nullable.(nullable)written at the end of a parameter tag (suitable for Ops written as Java classes, fields, and methods), recording that parameter as nullable.This PR additionally solves some other related issues:
Java Field Ops did not have their parameter types included in the YAML
This is a difficult issue for us to solve, because as annotation processing happens before compilation we cannot introspect the field type to find the functional method. The solution was to analyze the string output from the field's
TypeMirror, which includes fully qualified type strings for each of the type parameters of the functional interface. Since there's a direct mapping from type parameter to functional method type for each of the interfaces inscijava-function, it means we can derive parameter types for each Op field implementing one of the supported Op interfaces. If the field is not one of our supported interfaces, all parameter types are mapped to the StringUNKNOWN.Create a new
YAML-specificOpInfoimplementationYAMLOpMethodInfoto read Op parameters directly from YAML. This allows us to scrape parameter names and descriptions right away (instead of baking them in afterOpInfocreation)This change is not particularly necessary for solving Optionality of parameters is not embedded in Op YAML #200, but it is much easier to lump this change in with the fixes for it, because the in-javadoc nullable markers are not easily visible without this new
OpInfoimplementation. This new implementation is designed to lazily load allClasses relevant to the Op (the Op itself, parameter types, etc) as late as possible to save some computation, as well as to take advantage of the YAML data directly.TODO
* [ ] Update scijava-ops-image to utilize the new changes (e.g. declaring nullable parameters with the javadoc parenthetical instead of the annotation).Not going to do this right now, it's an internal change that doesn't need to be done right now.YAMLOpFieldInfoandYAMLOpClassInfo.* [ ] Consider anAlso doesn't need to be done right now.AbstractOpMethodInfoto house common functionality between currentOpMethodInfoandYAMLOpMethodInfo.scijava-ops-ext-parser?