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
data: define interface for
read_last_scalars
#6657data: define interface for
read_last_scalars
#6657Changes from 7 commits
f3dd50b
cb799f4
f30a9f8
1d8e79c
c78f31b
b069827
a10de2f
3d7e498
6400d75
822e1bf
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.
Since we're requiring plugin_name to be specified, I would think having the method be read_latest_values, or something like this, would be more generic/flexible, and we can say in the doc string that it's possible that not all plugins are supported by all implementations.
Although the return value does get a bit more difficult to specify and deal with, so if you prefer this because of this reason, I'm ok with 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.
I specified
plugin_name
becauseread_scalars
also requiresplugin_name
. I prefer the simpler definition for just the scalars, in this case do you think we can just remove theplugin_name
arg?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 decided to remove the
plugin_name
completely. I think it's unlikely that we'll have a new plugin type that contains scalar data in the near future. And since scalar seems to be the most used type, we don't have to generalize it toread_last_values
just yet, wdyt?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.
Oh, sorry for the late response. I thought I had replied to this. Technically, the plugin type is somewhat independent from the data type, e.g. there's a
custom_scalars
plugin in our examples, I believe, which also uses scalar values.For this reason, and to be consistent with the rest of the interface for the data provider, I'd lean a bit towards keeping the plugin_name... but this is ok too. I agree it's unlikely we'll ever need anything other than "scalars", and if we do, we can change it later (but again, unlikely).
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.
Great point, added it back.
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.
Also I didn't realize I also needed to implement this method in
event_processing/data_provider
. PTAL, thanks!