Skip to content

Tweak symbols in Op Descriptions#119

Merged
gselzer merged 4 commits into
mainfrom
scijava-ops-engine/tweak-symbols
Jan 25, 2024
Merged

Tweak symbols in Op Descriptions#119
gselzer merged 4 commits into
mainfrom
scijava-ops-engine/tweak-symbols

Conversation

@gselzer

@gselzer gselzer commented Jan 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

This PR makes slight changes to the default returns of OpEnvironment.help() and OpEnvironment.helpVerbose(), elimating smaller symbols (and the key describing them) in favor of Java-style annotations. Right now, that only means a @CONTAINER mark on any Computer output, and a @MUTABLE mark on any Inplace output.

This PR also consolidates down to two "describe" methods in the Infos utility class, as we added two new ones in #113 but did not remove any old ones.

Points of discussion:

  • As you can see in the associated test, the verbose descriptions for adapted Op infos are almost unusably long. How should we change them? For example:
org.scijava.ops.engine.matcher.adapt.OpAdaptationInfoTest.adaptableOp(java.lang.Double,java.lang.Double)|Adaptor:|Info:org.scijava.ops.engine.adapt.functional.FunctionsToComputers$Function2ToComputer2@0-SNAPSHOT{|Info:org.scijava.ops.engine.copy.CopyOpCollection$copyDoubleArray@0-SNAPSHOT{}}
    > input1 : java.lang.Double
    > input2 : java.lang.Double
    > output1 : @CONTAINER double[]

TODO:

  • Update documentation

Closes scijava/scijava#178 and scijava/scijava#179

@gselzer gselzer added the enhancement New feature or request label Jan 23, 2024
@gselzer gselzer requested a review from hinerm January 23, 2024 21:19
@gselzer gselzer self-assigned this Jan 23, 2024
@gselzer gselzer linked an issue Jan 23, 2024 that may be closed by this pull request
Update documentation scijava/scijava#179
Closed
@hinerm

hinerm commented Jan 24, 2024 •
edited
Loading

Copy link
Copy Markdown
Member
  • As you can see in the associated test, the verbose descriptions for adapted Op infos are almost unusably long. How should we change them?

You could indent each level of inheritance:

org.scijava.ops.engine.matcher.adapt.OpAdaptationInfoTest.adaptableOp(java.lang.Double,java.lang.Double)
      Adaptor: org.scijava.ops.engine.adapt.functional.FunctionsToComputers$Function2ToComputer2@0-SNAPSHOT
            Adaptee: org.scijava.ops.engine.copy.CopyOpCollection$copyDoubleArray@0-SNAPSHOT

Other thoughts:

  • Does the Info: add anything?
  • I don't think you need to list the version in the help output since you can't have multiple versions of an op loaded at runtime.
  • Does a user ever need to see package names? I can't think of a reason they would, off the top of my head. They are never going to use that information to call the op.
  • The parameter info in the op name (java.lang.Double,java.lang.Double) is redundant

@gselzer

gselzer commented Jan 24, 2024

Copy link
Copy Markdown
Member Author
  • As you can see in the associated test, the verbose descriptions for adapted Op infos are almost unusably long. How should we change them?

You could indent each level of inheritance:

org.scijava.ops.engine.matcher.adapt.OpAdaptationInfoTest.adaptableOp(java.lang.Double,java.lang.Double)
      Adaptor: org.scijava.ops.engine.adapt.functional.FunctionsToComputers$Function2ToComputer2@0-SNAPSHOT
            Adaptee: org.scijava.ops.engine.copy.CopyOpCollection$copyDoubleArray@0-SNAPSHOT

Well, keep in mind that the "name" used here comes from OpInfo.implementationName() currently, which is designed to give users some more information about which Ops exist. Maybe we should change that, though...what would you prefer to see @hinerm? Keep in mind this decision would affect all Ops, not just adapted Ops.

@hinerm

hinerm commented Jan 24, 2024

Copy link
Copy Markdown
Member

Well, keep in mind that the "name" used here comes from OpInfo.implementationName() currently, which is designed to give users some more information about which Ops exist. Maybe we should change that, though...what would you prefer to see @hinerm? Keep in mind this decision would affect all Ops, not just adapted Ops.

I would consider not changing the implementationName method but in verbose output processing looking at replacing |Info characters with newlines and indents.

I don't have any other suggestions on shortening the name beyond what I posted above. I fine just leaving it the way it is.

@gselzer
Update OpAdaptationInfo.toString()
0d08a00 
Much easier to read now!
@gselzer gselzer marked this pull request as ready for review January 24, 2024 20:13
@gselzer gselzer force-pushed the scijava-ops-engine/tweak-symbols branch from 3a99b75 to af57e2d Compare January 25, 2024 22:38
@gselzer gselzer merged commit 11135c4 into main Jan 25, 2024
@gselzer gselzer deleted the scijava-ops-engine/tweak-symbols branch January 25, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hinerm hinerm Awaiting requested review from hinerm

Assignees

@gselzer gselzer

Labels

enhancement New feature or request

Projects

No open projects
SciJava Ops Paper #1: Java
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update documentation Tweak how symbols are used in help

2 participants

@gselzer @hinerm