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.
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
ObjectType and Plugins #1787
ObjectType and Plugins #1787
Changes from 1 commit
a24ec60
ecf7e80
91bd98a
74aec62
ebda9d1
968ec24
bb638e6
fe8414f
e4da7bb
8ee1947
354a5a7
edc0617
ca1875d
781cbf2
9de9a46
44bec3b
18b66c1
a2dc389
2881aff
aa89bad
965337b
e12c2f8
f66297a
5e593e6
7dbc3b0
de91510
49577a0
3f0e0df
8d3b2e1
45970fd
2941ea9
5804556
b0afabf
14452e2
7265104
ce855b9
88d1686
a7fc020
d905b37
e342fe1
5c3beb2
7576357
8f4e8e3
9032e66
366151c
7bbf573
30e92c2
01c93e3
1dd9a59
17eea96
21a6185
4009b93
7681a3f
fe57ee7
eb9863c
1811bb3
33cf3d2
b298d47
cf36e5b
e492010
95ada5c
15296cb
d28c82d
a995b11
d736c85
580b272
ddfcdaa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
does this serve a purpose on its own? it seems like it just pulls in the plugin.ObjectTypesModule?
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've got a follow up PR for an application plugin; instead of refactoring the structure in the application plugin, I'm providing the follow-up structure for it here.
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.
"objectTypeCount=" perhaps? or just "count="?
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.
The logging at the end of the day is going to look like:
I'm not a fan of extraneously duplicating "count" in the log lines (in anticipation of more plugin types besides object type), but maybe it makes more sense to have a better name for the plugin type - ie, "ObjectType" to better infer
io.deephaven.plugin.type.ObjectType
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.
Is there a reason we couldn't have a single java type which itself is registered as a Plugin (via the service loader mechanism) and is as such automatically included in the java.Loader, provided it is on the classpath?
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.
That's a possible improvement in the future - will note ticket.
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.
Can any of these wiring classes live in their own gradle project? Most/all of the java/python loader code seems like it should be able to be external, and possibly only be loaded at runtime (for example, if python isn't even in use, skip it)
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.
That's a possible improvement in the future - will note ticket.