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

nitclk properties/functions that accept ViSession in C should only accept nitclk.SessionReference in Python #1042

Closed
marcoskirsch opened this issue Sep 18, 2019 · 5 comments

Comments

@marcoskirsch
Copy link
Member

Description of issue

In #1005 we agreed on the following:

Any place that needs a tclk session num in the C API can accept:

  • nimi-python Session (assuming NI-TClk support enabled)
  • nitclk.SessionReference
  • tclk session num

Any place that returns a tclk session num from the C API will return a nitclk.SessionReference

#1026 implements it.

I want to push back on this proposal for 3 reasons:

1. From Zen of Python:

There should be one-- and preferably only one --obvious way to do it.

If we implement the suggestion, then you can pass either [my_scope_session, my_digital_session] or [my_scope_session.nitclk, my_digital_session.nitclk] to nitclk functions.

2. From Zen of Python:

Explicit is better than implicit.

[my_scope_session.nitclk, my_digital_session.nitclk] is explicit.

3. Assymmetry in property set/get

An additional strange behavior introduced by the poposal is that on attributes of type nitclk.SessionReference, users can set with an niscope.Session but when the user gets it, it would be returned as nitclk.SessionReference. I think this is confusing.

@texasaggie97-zz
Copy link
Contributor

For ease of use, the first two options are useful in different places.

  • nimi-python Session - Useful in the list of session to pass to the API functions
  • nitclk.SessionReference - useful in the get/set attributes of type VISession since you do not know the type of the underlying driver in order to recreate the nimi-python Session

I think it would be bad to take one type for the functions but require a different type for the get/set ViSession attributes, so we should take either in both places.

Taking the integer nitclk session number would be a back door in case there is something we missed, however, requiring the creation of nitclk.SessionReference in order to use nitclk is not that bad.

@marcoskirsch
Copy link
Member Author

Note:
If we go with the proposale here and only support nitclk.SessionReference then I think #1024 becomes very difficult to implement.

@marcoskirsch
Copy link
Member Author

Taking the integer nitclk session number would be a back door in case there is something we missed, however, requiring the creation of nitclk.SessionReference in order to use nitclk is not that bad.

Let's say someone is putting together their own NI-RFSA Python bindings because #984 and that user wants to use nitclk module. The user will have a ViSession vi somewhere in there and maybe that's why we're justifying having nitclk accept int for session reference. But that user will possibly want to set NI-TClk properties on their session reference... so we will need to provide a way to construct an nitclk.SessionReference regardless.

@marcoskirsch
Copy link
Member Author

We discussed this at length in today's code review.
We agreed to:

  • Remove support for raw numbers
  • Support passing either Session or SessionReference. This is true for attributes too.
  • Attribute reads will return SessionReference. This is weird because there is a potential assymetry but we will leave for it for practicality.

@texasaggie97-zz
Copy link
Contributor

Closed by #1026

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

No branches or pull requests

2 participants