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
Define a IM/DM decoupling interface for interaction model engine to interact with the data model bits #32914
Define a IM/DM decoupling interface for interaction model engine to interact with the data model bits #32914
Changes from 55 commits
5697b48
2a59741
d23acfa
67c6d29
a582d72
efa1afe
4eeb04f
f4c3b46
4abcb5e
34b646c
02bc810
05a085a
dfab9de
43308e8
fa4cfa8
7fb1a3c
e97254b
f855e93
0aa318a
7e95ee7
6e532a6
4c3391a
0a0b315
eb58bc9
d88fb0f
acc842c
6886d0c
6c805d1
bc5f9f6
b9db782
24176e9
cff6de1
7310b65
7d99ad5
c363ddd
c925cb4
72ea3cc
1da1c87
036782f
ddbe6a6
f3673ba
b220cf3
c2d2935
af649e2
0c9cd9e
7856b2e
e9c0039
e6cdc3e
7a12efd
8c86e3d
e4b0010
533d542
4be818f
492b6f9
5f76b9c
7e18454
5844827
e5e97a8
89c5bcf
ceea7b8
d134fab
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.
Here's a problem: "Action" in the context of the interaction model has a specific meaning, and this struct has absolutely nothing to do with that meaning.
I can't tell by looking at this struct name, or the struct definition, what is supposed to go into here and why it has, say, Events but not Attributes or Commands. And I have no idea what Paths is supposed to be. If it's "things you can do with paths", that seems like the wrong level of abstraction to me.
This is really "InteractionModelAPIs" or something. But then broken down into separate structs for separate types of APIs... (more on this below).
Realistically, this should perhaps have been called
chip::app::InteractionModel
(as a class, not a namespace), but if we are using that name for the namespace....chip::app::InteractionModel::Instance
?chip::app::InteractionModel::Callbacks
? Something else? I am struggling to find a sensible name 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.
So this class name... Is this intended to be a grab bag of "all APIs related to events for a given IM implementation" or something?
The naming here is pretty opaque as an API consumer; what does it mean to have an
Events *
that does not actually give me access to any events, for example?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 had a hard time naming overall. What I have is that various things made their way into our data model implementation in ember without explicitly being named or sorted in categories - they moved organically as "we neeed this" and it seems to typically carry over either as internal access (e.g. exchange-context -> session -> groupid) or as "implied global" (this is what events do) and this is what makes decoupling extra hard - I really don't want to use implied globals.
I tried to split them as:
I give up ... some code really needs access to internals, specifically GetExchangeContext
I would welcome suggestions of good names. This seems to be some
EventContext
however naming everything Context seems odd ... and I am torn about context being a filler word or not.Maybe the correct thing would be to place these under a
Context
orCallback
namespace, however since I could not find a good name I changed it to "ok for now". Willing to update though.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 think "paths" is very weird because there are lots of types of paths. Do "event paths" go under Paths or under Events?