Refactor descriptions#113
Merged
Merged
Conversation
gselzer
added
documentation
gselzer
force-pushed
the
scijava/scijava-ops-engine/refactor-descriptions
branch
from
402bd33 to
3f2fa69
Compare
gselzer
commented
Jan 12, 2024
gselzer
left a comment
gselzer
left a comment
Member
Author
There was a problem hiding this comment.
I like the changes, but as expected with a draft PR it's sloppy
Comment on lines
-22
to
-26
| public final class OpWrappers { | ||
|
|
||
| private OpWrappers() { | ||
| // Prevent instantiation of static utility class | ||
| } |
Member
Author
There was a problem hiding this comment.
Was this not a part of the OpWrappers.vm file?
| var infos = SimplificationMatchingRoutine.getInfos(env, req.getName()); | ||
| var filtered = filterInfos(infos, req); | ||
| String opString = filtered.stream() // | ||
| .map(Infos::describe) // |
Member
Author
There was a problem hiding this comment.
We should consider deleting unused methods - there are now 4 different Infos.describe... methods - decide whether we need/want all of them
Comment on lines
+149
to
+157
| return describe(info, null, null); | ||
| } | ||
|
|
||
| /** | ||
| * Generates a {@link String} describing the given {@link OpInfo} | ||
| * | ||
| * @param info the {@link OpInfo} of interest | ||
| * @param name the name that the Op was searched by | ||
| * @return a descriptor for {@code info} |
Member
Author
There was a problem hiding this comment.
Again, figure out how much of this is needed
Comment on lines
59
to
65
|
|
||
| String expected = "test.classDescription(\n\t " // | ||
| + "Inputs:\n\t\tjava.lang.Double input1\n\t\tjava.lang.Double input2\n\t " // | ||
| + "Outputs:\n\t\tjava.lang.Double output1\n)\n"; | ||
| + "Output:\n\t\tjava.lang.Double output1\n)\n"; | ||
| String actual = info.toString(); | ||
| Assertions.assertEquals(expected, actual); | ||
| } |
Member
Author
There was a problem hiding this comment.
We may want to switch info.toString() to the current Infos.describeMultiline if we like that better
Comment on lines
+137
to
+138
| org.scijava.ops.engine.stats.Mean, | ||
| org.scijava.ops.engine.stats.Size; |
| @@ -99,7 +99,7 @@ public class DefaultTubeness<T extends RealType<T>> implements | |||
| private Function<Dimensions, ImgFactory<DoubleType>> createFactoryOp; | |||
|
|
|||
| //TODO: make sure this works | |||
| */ | ||
|
|
||
| package org.scijava.ops.image.slice; | ||
| package org.scijava.ops.image.transform.slice.slice; |
Member
Author
There was a problem hiding this comment.
Why is this package slice.slice?
| .producer(); | ||
|
|
||
| Function<Class<T>, T> typeFunc = ops.unary("create") // | ||
| Function<Class<T>, T> typeFunc = ops.unary("create.object") // |
Member
Author
There was a problem hiding this comment.
Consider coming up with a better name...
| */ | ||
|
|
||
| package org.scijava.ops.image.project; | ||
| package org.scijava.ops.image.transform.project.project; |
Member
Author
There was a problem hiding this comment.
This package should not have two project
| Assertions.assertEquals(expected2, typeAssigns2); | ||
| } | ||
|
|
||
|
|
hinerm
force-pushed
the
scijava/scijava-ops-engine/refactor-descriptions
branch
2 times, most recently
from
0e8c715 to
88d5626
Compare
We're not doing any kind of fuzzy matching at this point.
equals is too strict.. we just want a startsWith
Since aliases are no longer returned in help text, many of these tests require two separate classes to be registered in a bare op environment.
hinerm
force-pushed
the
scijava/scijava-ops-engine/refactor-descriptions
branch
from
43256f7 to
baee2b5
Compare
hinerm
approved these changes
Jan 19, 2024
2 tasks
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 is (currently) a messy attempt at making the return from
DefaultOpEnvironment.help()a lot cleaner to read. We make the following broad changes:engine.adaptOps now use theengine.createandengine.copynames where necessary for adaptation purposes. Other Ops were altered as needed to enforce this requirement.*and^, respectively. @ctrueden liked this the best.Note that this PR is designed to close scijava/scijava#167, and is meant to give @hinerm a starting point since he assigned himself that issue.
Points for further discussion:
OpInfos harder, because right now we only collapse based on equalStrings. But, if the names are scraped from javadoc, we can't stop someone from e.g. making a typo and adding a new entry to the help.I never changed the verbose description - how should that change, if at all?With @elevans' help, I edited the verbose descriptions to look similar to the simple descriptions, but they also include type/implementation/description details.TODO: