-
Notifications
You must be signed in to change notification settings - Fork 19
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
Handle sensor list in topic #154
base: main
Are you sure you want to change the base?
Handle sensor list in topic #154
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
+ Coverage 91.23% 91.26% +0.02%
==========================================
Files 32 32
Lines 4417 4429 +12
==========================================
+ Hits 4030 4042 +12
Misses 387 387
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 8799660697Details
💛 - Coveralls |
pytroll_collectors/scisys.py
Outdated
"""Update sensor tuples and lists to MULTIPLE_SENSORS.""" | ||
to_send_dict = to_send.copy() | ||
if "sensor" in to_send: | ||
if type(to_send["sensor"]) is list or type(to_send["sensor"]) is tuple: |
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 it is perhaps more pythonic to do isinstance(to_send["sensor"], collections.abc.Sequence)
?
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.
isinstance(to_send["sensor"], (list, tuple))
should also work
pytroll_collectors/scisys.py
Outdated
to_send_dict = to_send.copy() | ||
if "sensor" in to_send: | ||
if type(to_send["sensor"]) is list or type(to_send["sensor"]) is tuple: | ||
to_send_dict["sensor"] = "MULTIPLE_SENSORS" |
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.
Should it be "multiple sensors" rather?
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 also prefer lower case
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.
Apart from a small comment it looks good, thanks!
Unfortunately |
In case of list of sensor, use MULTIPLE_SENSORS in topic. We do not want a tuple or lsit in the publishing topic.