Skip to content

Move op index & others#21

Merged
ctrueden merged 4 commits intomasterfrom
move-op-cache
Sep 22, 2019
Merged

Move op index & others#21
ctrueden merged 4 commits intomasterfrom
move-op-cache

Conversation

@wiedenm
Copy link
Member

@wiedenm wiedenm commented Jun 21, 2019

This PR comprises a set of rather loosely related changes. The main topics are:

Feel free to approve or reject these changes separately.

The PR does not yet deal with the removal of all parts of the plugin system. OpService and TypeService still exist and use other services internally. In particular, OpService still relies on PluginService to collect Ops and Transformers.

@wiedenm
Copy link
Member Author

wiedenm commented Jun 21, 2019

The diffs of DefaultOpTypeMatchingService and DefaultOpTransformerService got polluted by formatting and might be a bit hard to read, sorry about that. I should be using the eclipse preferences from imagej/imagej - are those the right ones?

@wiedenm wiedenm changed the title Move op cache Move op index & others Jun 21, 2019
@ctrueden
Copy link
Member

ctrueden commented Jun 22, 2019

I should be using the eclipse preferences from imagej/imagej - are those the right ones?

You and @gselzer should just stop autoformatting on save. It creates tons of problems. We can try to fix the autoformatters but it is low priority. If you want to format a code block that you just wrote, select it and then shift+ctrl+F it.

It is possible to split these muddy commits using git rebase -i with some care. I can show you a couple of approaches I use early next week, if you like.

@wiedenm
Copy link
Member Author

wiedenm commented Jun 22, 2019

Yes, I usually turn off save actions when working on already existing files and only use them for new files, forgot it here. I know about Shift+Ctrl+F, I mostly use save actions for clean-ups (making variables final etc.). It's a pity that there's no Shift+Ctrl+F equivalent for that.

Sounds good, thanks!

@ctrueden
Copy link
Member

ctrueden commented Jun 22, 2019

It's a pity that there's no Shift+Ctrl+F equivalent for that.

For Clean Up, I usually type (on macOS): option+command+S and then C,C,return. Yeah, no official shortcut, but it's still quite fast. Unfortunately, I believe Clean Up is always global to the file, rather than operating on the current selection, so it will not help you here. This is why you see me adding final keywords by hand sometimes.

@wiedenm wiedenm force-pushed the move-op-cache branch 4 times, most recently from 90951d1 to 09ad613 Compare June 28, 2019 16:24
@wiedenm
Copy link
Member Author

wiedenm commented Jun 28, 2019

Cleaned up the diffs some more, the PR should be ready for review now.

wiedenm and others added 4 commits September 22, 2019 17:53
Trims down the interface to a collection of Ops that can be listed and
queried by name.
This was an artifact of the old imagej-ops that does not hold true in
the new scijava-ops implementation.
This also removes the notion of a "canonical" Op name. From now on, all
names that are specified for any given Op are treated equally.

Note that this commit also fixes #20. It is however questionable
whether the fixed behavior is desired at all. Might be adjusted in a
future commit.
From OpTypeMatchingService and OpTransformerService, and rename them
accordingly.

This commit also separates indexing transformers from matching them by
moving the indexing logic to OpService.

Co-authored-by: Curtis Rueden <ctrueden@wisc.edu>
@ctrueden
Copy link
Member

Darn, I completely forgot about this PR, and since then work continued on the migrate-imagej-ops branch, with @gselzer redoing some of the work you had already done here.

I have now reconciled the branches, fixing conflicts, etc. Some of the work here might was lost: in particular, the separation of the op index into its own OpIndex class. But because the op index was still simplified to a map instead of a tree data structure, it is not so terrible leaving it in the OpService for now. We should still separate it at some point soon. Also, the goal of keeping the iteration order consistent and sorted is probably not met now—@gselzer just used a normal HashMap rather than anything fancy like TreeMap (always sorted) or LinkedHashMap (preserves insertion order).

Anyway, all commits compile with passing tests, and are still a step forward, so I'll merge it now.

@ctrueden ctrueden merged commit b52c0bd into master Sep 22, 2019
@ctrueden ctrueden deleted the move-op-cache branch September 22, 2019 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

2 participants