Skip to content

Conversation

@gselzer
Copy link
Member

@gselzer gselzer commented Jan 14, 2022

Fixes to #46.

gselzer added 19 commits July 1, 2022 16:21
None of the stuff it describes is needed anymore. Note that it is now
possible for both Simplification.IN_PROGRESS and
Simplification.FORBIDDEN to exist in the same Hints object. This should
be fine
It is only used in one place, and it is only one line.
This way, IDEs make the connection between this reference and the field
This removes "an op" and "{@link Op}" from the codebase
This required we delete the PluginBasedClassOpInfoGenerator, since the
Op interface no longer extended SciJavaPlugin. But that's fine, since we
were going to delete that anyways!
Also, changed the line endings in OpInstance from Windows to Linux.
We were conflating too many things with the name op()
Don't confuse people with the flavoring
@ctrueden ctrueden force-pushed the scijava/scijava-ops-engine/data-provenance-revisions-revisions branch from 98850ae to d2f2aba Compare July 1, 2022 21:26
Copy link
Member

@ctrueden ctrueden 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. A couple of small comments, and some commit messages do not use imperative tense as is standard, but this is good to go!


static final String IMPL_DECLARATION = "|Vanilla:";
/** Identifier for an unaltered OpInfo in an Op signature **/
String IMPL_DECLARATION = "|Info:";
Copy link
Member

Choose a reason for hiding this comment

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

Some reason this shouldn't be |Op: instead?

@@ -1,53 +1,97 @@

Copy link
Member

Choose a reason for hiding this comment

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

A bunch of files in the repo have been Windows line endings up till this point... but now they are getting converted over one by one. This is much better done as its own commit fixing all of them at once, when this type of problem is noticed. Otherwise, the diff here is obfuscated by the line ending shift.

* Creates an Op instance from an {@link OpInfo} with the provided
* {@link MatchingConditions} as the guidelines for {@link OpInfo} selection.
* This Op instance is put into the {@code opCache}, and is retrievable via
* This Op instance is put into the {@link #opCache}, and is retrievable via
Copy link
Member

Choose a reason for hiding this comment

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

By default when you generate javadoc, private members are not included in the generated output. So then this sort of link, which goes to a private field, will not actually be functional. I am actually not sure how this ends up rendering in the default case, but we should check. If it looks bad or confusing, we can just remove the @link in favor writing "op cache" in normal language. But if it looks OK, I favor leaving it so that in IDEs the linkage is still there.

@ctrueden ctrueden merged commit a00c198 into main Jul 1, 2022
@ctrueden ctrueden deleted the scijava/scijava-ops-engine/data-provenance-revisions-revisions branch July 1, 2022 21:44
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