-
Notifications
You must be signed in to change notification settings - Fork 6
Initialize scijava-struct as a new module #31
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
Conversation
21b4d4e to
dcc7df2
Compare
dcc7df2 to
bf2a68b
Compare
bf2a68b to
d090c1c
Compare
This commit removes the org.scijava.struct package, since it was moved to its own module. scijava-ops now depends on scijava-struct
04718c1 to
f6df62f
Compare
ctrueden
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this code together with Christian over many painstaking iterations at a hackathon, so it's not surprising that I'd be satisfied with it as is. But I am. Satisfied. 😆
| <mailingLists> | ||
| <mailingList> | ||
| <name>Image.sc Forum</name> | ||
| <archive>https://forum.image.sc/tags/scijava-struct</archive> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be /tag/scijava.
| </mailingLists> | ||
|
|
||
| <scm> | ||
| <connection>scm:git:git://github.com/scijava/incubator</connection> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be scm:git:https://github.com//scijava/incubator since GitHub stopped supporting git://.
| </issueManagement> | ||
| <ciManagement> | ||
| <system>Travis CI</system> | ||
| <url>https://travis-ci.com/scijava/incubator</url> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to change to GitHub Actions.
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class DefaultStructInstance<C> implements StructInstance<C> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely javadoc the struct stuff. Of course we need javadoc on all public API, but struct is super abstract and will be very helpful to have it documented for later.
| * result. | ||
| * | ||
| * @see org.scijava.param.Mutable | ||
| * @see org.scijava.ops.function.Inplaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct does not depend on any scijava ops components, right? So this @see is not valid. It's great to point out that MUTABLE is relevant to inplace functions in ops, but I think we need to do it in a "non-type-safe" way, unfortunately.
| * but whose content will be populated during execution of the operation. | ||
| * | ||
| * @see org.scijava.param.Container | ||
| * @see org.scijava.ops.function.Computers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This @see may not be valid; see below.
| * | ||
| * @see Field#getGenericType() | ||
| */ | ||
| // TODO: Use Type<T> or Nil<T> from new scijava-types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO is obsolete now, right? Or should this be Nil<T>?
| default MemberInstance<T> createInstance( | ||
| @SuppressWarnings("unused") Object o) | ||
| { | ||
| return () -> Member.this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My brain is probably not working today, but... what even is this syntax‽
Edit: OK, I get it... it's (mostly) equivalent to:
Member<T> me = this; return new MemberInstance<T>() { @Override Member<T> member() { return me; } }but in slick lambda shorthand.
This PR introduces
scijava-struct, a new module extracting theStruct/Memberdata structures from the present Ops module. Partitioning these data structures into their own module allows us to unify logic between SciJava Ops and KNIME (see scijava/scijava#58)TODO:
implpackage that is notexported? Can we manage this with Enforce better package relationships at build time scijava#62 in mind?Member(namely, adding adescriptionField); therefore, this PR is built on top of it