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

fix(compact sensor): correctly parse alphabetic id string modifier #7

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

richardstephens
Copy link
Contributor

This should be a simple one, but I have two questions:

  1. What do you think about refactoring these to use binary literals instead of hex literals for pulling out the right bits?
  2. I don't love the number of cases under which these parse functions panic - Should they return a Result<> instead?

@datdenkikniet
Copy link
Owner

  1. Binary literals are definitely better for understandability, so I think that's a good idea.
  2. Yeah, I originally went with Option<T> because it was just easier to get going. Returning actually descriptive errors is absolutely on the roadmap, but has not been priority for now.

LMK if you would like this PR merged as-is, or if you want to add more changes before doing so.

@richardstephens
Copy link
Contributor Author

I've added a changelog entry to this PR and rebased on master.

I'm looking at implementing some of the other SDR types but that could take a bit longer so I think it makes sense to merge this as-is.

BTW, I sent you an email about test hardware :)

@datdenkikniet datdenkikniet merged commit 2564d75 into datdenkikniet:main Jan 19, 2024
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