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
Shared communicator #652
Shared communicator #652
Changes from all commits
951cc3a
7f1bd36
c806571
11d2053
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Feels a little weird to put this in the
common
subpackage. This feels like it makes a fair number of assumptions about how it will be used and should probably live closed to where it is being used, at least for now. Thoughts?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 have had same thought about a lot of the new things in this set of changes and other similar ones involving dmod.core. One the one hand, we want to be able to reuse things. On the other, we don't want to pack too much into a "core" package that isn't really fundamental to what DMOD does. And on the third hand, too many library packages doesn't make sense either.
If all these new things are general enough to belong in the dmod.core package, then, given how large this PR is, I'd like to see the dmod.core changes (or at least the bulk of them) broken out into a dedicated PR. That can be handled first in isolation and will make things much easier to review and discuss effectively.
There may also be other similar points of decoupling - e.g., perhaps GUI-related changes - that can also be separated into individual PRs to further help make all of this easier to digest.
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 point is to have general tools. Sticking these structures within a purpose built library makes it more difficult to find useful shared tools. If I buy a spark plug wrench and keep it in my truck, I probably won't remember I have one when it comes time to work on my lawn mower.
A
Bag
data structure or a map that triggers events don't bear any tight coupling to specific actions, either. AnEventfulMap
doesn't provide any functionality that performs operations like evaluations, but functionality that helps perform evaluations specifically can leverage it for easier object management operations.A
dmod.common
library would work well to keep functionality separate. As it stands today,dmod.core
is the only python library that presents itself as general or cross-functionality purposed.There are multiple issues when it comes to breaking out changes to common code and putting that in a separate PR:
Putting isolated tasks into their own PRs makes sense - if I'm specifically going in to add in a new feature or add in a bug fix or add documentation, a PR for each makes sense. Breaking off utilities into their own PRs does not provide that same benefit.
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 totally agree with having general tools centralized for easy reuse. And part of what I was trying to say was that I don't want to add another library package. But I'm having trouble wrapping my head around the idea of tools that are general but make no sense without context.
The relationship you're describing sure sounds like things that are tightly coupled, in which case they'd probably belong in the same package. I'm not saying they do, but they should be separable otherwise (or at least the one not dependent on the other should be).
I'll think a bit more about this over the weekend; I don't want to make things more complicated just from trying to reducing complexity elsewhere. But I am a little worried about the notion of not being able to separate these things (as opposed to it not being worth the trouble).
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.
Maybe we can land somewhere in the middle? The majority of the features introduced in the PR could make up a entire library or framework. I think we can and should use python's subpackaging semantics to make that clearer than what we have now. Im thinking we create
dmod.core.multiprocessing
ordmod.core.rpc
and move the majority of this work into that subpackage. As for naming, im just throwing ideas out there, i'm certain there are better names that what I just suggested.The main point of this is to group code in submodules that capture the expressed concepts and keep related or dependent concepts close to one another (
A
needsB
soA
is close toB
). I hear your point about classes like aBag
, but I think the classes added in this PR are more specialized and arent really abstract data types like a bag is. Thoughts?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.
Now with a little more understanding of how all the parts fit together, why can't this be a
Communicator
. From what i'm seeing, anOccurrenceTracker
subtype will always be on the server side (e.g. it will track calls made through a prototype).