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

Read device ID from type metadata #125

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Read device ID from type metadata #125

merged 1 commit into from
Jul 16, 2024

Conversation

glopesdev
Copy link
Collaborator

The current implementation of GetDeviceContext and GetPassthroughDeviceContext explicitly receives the device ID as a numerical argument. While functionally sound, this approach makes it harder to generate informative error messages, since there is no way to map the ID back to a device name.

This PR refactors this approach to instead receive the expected device type. The numerical device ID is then extracted directly from the type metadata by searching for a literal field with the name ID. If no such field is found, or a device with an invalid ID is detected, the exception will now contain the type name.

The change also establishes parity between ContextHelper.GetDeviceContext and DeviceInfo.GetDeviceContext as they now both receive types as arguments.

The main caveat of this approach is that now declaring a static class with a const ID field is no longer optional. Since the approach relies on type metadata, the compiler cannot enforce the compliance of device types with this rule at compile-time. This is probably minor as any errors or inconsistencies will immediately throw errors on any test. The most significant impact will be refactoring the name of this field in the long-term, which would have to be done with care.

Fixes #116

@glopesdev glopesdev added feature New planned feature fix Pull request that fixes an issue labels Jul 2, 2024
@glopesdev glopesdev added this to the 0.1.0 milestone Jul 2, 2024
@glopesdev glopesdev requested review from jonnew and aacuevas July 2, 2024 21:03
Copy link
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, much more helpful.

@jonnew jonnew merged commit 7c88820 into main Jul 16, 2024
glopesdev pushed a commit that referenced this pull request Jul 19, 2024
Read device ID from type metadata
@glopesdev glopesdev deleted the issue-116 branch July 30, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New planned feature fix Pull request that fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve exception messages
2 participants