Skip to content
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 more SDR record types #10

Closed
wants to merge 3 commits into from

Conversation

richardstephens
Copy link
Contributor

Creating as a draft PR as there is still some work to do.

Questions:

  • 'event-only' sensors are missing some of the fields in SensorRecordCommon. There are a few options for handling this.
    • remove those fields from common
    • make them optional in common
    • break common into two types, one for Full+Compact, and one for all three

Still todo:

  • tidy up interfaces to be consistent
  • make parse methods return Result<> instead of Option<>
  • refactor out duplicated common parsing code

@datdenkikniet
Copy link
Owner

datdenkikniet commented Jan 27, 2024

'event-only' sensors are missing some of the fields in SensorRecordCommon

I think removing the fields from SensorRecordCommon so that we can have one in all three EventOnly, Full, and Compact makes the most sense. Since the idea is really that it can identify/be used for any 'ole sensor, it shouldn't include fields that aren't actually common to all sensor records.

We could put some of the common-to-full-and-compact-sensors things in another struct, but since that's only two or three fields I propose that we just add those fields to the structs for now.

@datdenkikniet
Copy link
Owner

Figured I would check in to see if you are making progress on this/would like to finish this (eventually), or if you're OK with me making some of the changes and merging it in in some form?

This functionality is super nice, it adds a super nice "completeness" feeling to the get-info example :P

@richardstephens
Copy link
Contributor Author

We’re actually running these commits in our fork pretty-much as is.

I will definitely get back to this eventually, but at this stage I’m not sure when that will be, so if you’d like to run with this that’s cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants