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

Laser's missing AbstractClass implementation #180

Open
edyoshikun opened this issue Sep 6, 2023 · 2 comments
Open

Laser's missing AbstractClass implementation #180

edyoshikun opened this issue Sep 6, 2023 · 2 comments
Assignees

Comments

@edyoshikun
Copy link
Contributor

The Laser's are still missing the abstract class implementation.

Commands that should be here:

  • def laser_on():
  • def laser_off():
  • def set_power():
  • def set_cw_mode():
  • def set_digital_modulation_mode():
  • def check_laser_status():
This was referenced Sep 6, 2023
@ieivanov
Copy link
Collaborator

ieivanov commented Sep 9, 2023

I'm finding the implementation of properties like this a bit confusing:

@property
def pulse_mode(self):
    """
    Toggle Pulse Mode On and Off (1=On)
    """
    self._pulse_mode = self._write_cmd('?PUL')[0]
    return self._pulse_mode

@pulse_mode.setter
def set_pulse_mode(self, mode=0):
    """
    Toggle Pulse Mode On and Off (1=On)
    """
    self._pulse_mode = self._write_cmd('PUL', str(mode))[0]

Based on the name, I would expect set_pulse_mode to be a function taking mode as an argument, for example:

laser.set_pulse_mode(mode=1)

Instead, set_pulse_mode is a property setter and needs to be used as:

laser.set_pulse_mode = 1

I would suggest renaming set_pulse_mode to pulse_mode as in:

@property
def pulse_mode(self):
    """
    Toggle Pulse Mode On and Off (1=On)
    """
    self._pulse_mode = self._write_cmd('?PUL')[0]
    return self._pulse_mode

@pulse_mode.setter
def pulse_mode(self, mode=0):
    """
    Toggle Pulse Mode On and Off (1=On)
    """
    self._pulse_mode = self._write_cmd('PUL', str(mode))[0]

such that the interaction would look like this:

>>>laser.pulse_mode
"0"
>>>laser.pulse_mode = 1
>>>laser.pulse_mode
"1"

An example of this is described here: https://realpython.com/python-getter-setter/#using-properties-instead-of-getters-and-setters-the-python-way

@edyoshikun
Copy link
Contributor Author

@ieivanov yes, this makes sense. I also agree that having the functions be called set_function_name for the setters is redundant.

@edyoshikun edyoshikun self-assigned this Sep 9, 2023
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

No branches or pull requests

2 participants