-
Notifications
You must be signed in to change notification settings - Fork 21
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 support for allow_version_mismatch (L1-2516) #287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to introduce this flag with default True
? If I am not mistaken the idea is that when its first released the flag will be False
in zhinst.core ... So wouldn't it make more sense to have the default set to False
in toolkit already?
src/zhinst/toolkit/session.py
Outdated
# Attempt to pass the allow_version_mismatch flag. Fallback in case | ||
# zhinst.core does not support it yet. | ||
def _create_daq( | ||
self, | ||
server_host: str, | ||
server_port: int, | ||
allow_version_mismatch: bool, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Attempt to pass the allow_version_mismatch flag. Fallback in case | |
# zhinst.core does not support it yet. | |
def _create_daq( | |
self, | |
server_host: str, | |
server_port: int, | |
allow_version_mismatch: bool, | |
): | |
def _create_daq( | |
self, | |
server_host: str, | |
server_port: int, | |
allow_version_mismatch: bool, | |
): | |
"""Create a new session to the data server. | |
Attempt to pass the allow_version_mismatch flag. Fallback in case | |
zhinst.core does not support it yet.""" |
My idea was to do a two stages release. First with the flag present but disabled by default, the a second commit that "flips" the condition to enabled by default. For toolkit, maybe I can merge the "flipping" commit within this same PR |
0ebc513
to
9c9a47b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
||
@staticmethod | ||
def _create_daq(server_host, server_port): | ||
try: | ||
return ziDAQServer(server_host, server_port, 6, allow_version_mismatch=True) | ||
except TypeError as error: | ||
if "allow_version_mismatch" not in error.args[0]: | ||
raise | ||
return ziDAQServer(server_host, server_port, 6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: dead code right?
2dff201
to
6f92343
Compare
Expose the flag so that once it's changed to False by default, it can be overridden.
6f92343
to
6c16f7d
Compare
Expose the flag so that once it's changed to False by default, it can be overridden.
Description:
Fixes issue: #
Checklist: