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
Add measurement processing workflow #363
Add measurement processing workflow #363
Changes from all commits
3ea6d35
589ae59
0719bd0
7f4e645
e3db4fd
036381a
8e42c05
e7fe93a
8946cef
bea61a4
d00b481
49d5181
83af3ef
6a4f5d9
feddbbf
89cbe73
d430886
3c55c8a
2a22f81
1ed4c8c
ccc161d
4e5ed41
0ae5513
e954a78
61cc365
4b19826
f04263a
b400d2d
5106b30
98815b6
88cbda8
ad91d94
69cf9da
863d8ea
66599df
e75b00b
6e3d743
a1a5a48
a19056e
d3415da
fb29c4a
9781d26
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.
To make sure that I understand correctly, this
sensor_type.processing_formula
is expected to be one of0, 2, 4, 6
?If so, how is this value guaranteed to be one of them?
My guessing is that our server somehow sends a user a list of available numbers (=0, 2, 4, 6) and there's no way for a user to choose a value other than the one in the list. But I couldn't find any link from the key ofprocessingFomula
dictionary (0, 2, 4, 6).Could you explain this?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 found that there is
built_in_content.py
and alsoag_data/presets/sample_user_data.py
. I guess the implementation of the builtin function and alsoprocessingFormula
dictionary should be inside the file.And there are total 3 files that hardcode 0, 2, 4, 6, which makes maintenance harder. For instance, when you add a sensor with index 10, then you should edit three files (Shotgun surgery) and it's not even easy to find which files to edit.
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.
This is a fast and cheap but not good implementation. Ideally, the mapping should be robust and easy to change, so the user or developer does not need to maintain the digits in different yet not directly linked files to modify the code and still make it work. I am thinking what Dan has been saying, that a string or other version of mapping may be more appropriate.
The problem here is, how do we map something defined in a dictionary (maybe a value of some dictionary-allowed type, such as numbers, strings, bool, etc) to a function handler?
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.
It's obvious that we should have this kind of in-memory dictionary that maps something to a function in our python code unless we dynamically evaluate a raw string from outside of our codebase hoping that it's a valid python code snippets.
I don't really want to evaluate an untrusted code. Providing a sandboxed environment for it is tricky especially for python.
So the conclusion is, the way that the current code works is good as-is but let's organize it for us not to edit multiple locations when we work on 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 feel this topic is bigger than what we can accommodate in the discussion of a PR. So here is a dedicated issue #378 created for reimagining the formula function mapping.