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

Discussion: Have an enum for Device Identifiers #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NobodysNightmare
Copy link
Contributor

Hi there,

this Pull Request is (at least initially) intended as a discussion point. I have no big feelings that you will eventually merge this.

What is it?

A small change, introducing an enum for the device identifiers:

public enum DeviceIdentifier
{
    BrickletAmbientLight = 21,
    BrickletAnalogIn = 219,
    // ...SNIP...
    BrickServo = 14,
    BrickStepper = 15
}

This enum is used in the Enumerate-Callback instead of an integer. It should also be used in GetIdentity, however this involves more changes to the generators and I would only do this if you think its worth it.

Why?

Thread 1
Thread 2
Thread 3

Second thread is for C, but it also shows the explorability issues associated with that. Having a DeviceIdentifier adds some benefits:

  • more type safety (it's C#)
  • better explorability
    • using an IDE you get auto-completion for DeviceIdentifiers
    • using no IDE you have a clear hint where to look (DeviceIdentifier tells you there is a type you can look up, int tells you nothing, you have to guess)

Why only C#?

In my opinion this should be included in all language-bindings that feature type-safety and where enums are usual. Best example is Java.
However for the discussion one example is enough.

Why not Merge?

I think you already realized why you might not want to merge that: Backwards-compatibility.
Merging this request would break current implementations of enumerate-callbacks, because they now have an DeviceIdentifier-parameter instead of an int-param. However, it would clearly not impact the hardware-compatibility.

I might have proposed this exact change earlier, but back then I did not dive into the code deep enough.

What do you think of this proposal?

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.

1 participant