Skip to content

Remove assert from __eq__ for windows #504

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

Closed
m1guelperez opened this issue Oct 23, 2023 · 5 comments
Closed

Remove assert from __eq__ for windows #504

m1guelperez opened this issue Oct 23, 2023 · 5 comments

Comments

@m1guelperez
Copy link
Contributor

m1guelperez commented Oct 23, 2023

for window in session.list_windows():
    if window.name == "test_window":
        target_window = window
        break
    if target_window == None:
        target_window = session.new_window(attach=True, window_name="test_window")
    target_window.select_window()
    target_window.panes[0].send_keys("exec zsh")

The code above will crash, when the window exists and we then check the if target_window == None because of:

def __eq__(self, other: object) -> bool:
    assert isinstance(other, Window)
    return self.window_id == other.window_id

I guess it would be better to remove the assert and return false. Should I do a PR?

@tony
Copy link
Member

tony commented Oct 23, 2023

Can you summarize / explain the code snippet a bit more? I formatted the code

. Should I do a PR?

Yes. If you make a PR, I can examine it closer.

@m1guelperez
Copy link
Contributor Author

m1guelperez commented Oct 23, 2023

Consider you iterate through all existing windows, to see if a window with a particular name already exists.
We basically use target_window here as placeholder. If the window we search does not exist it will be None.

However, in the window.py file, the == is implemented as the code I provided. But I do not think that there should be an assert.

The method to check equal for windows should be implemented like that:

def __eq__(self, other: object) -> bool:
    if not isinstance(other, Window):
        return False
    return self.window_id == other.window_id

Is it clearer now?

@tony
Copy link
Member

tony commented Oct 23, 2023

I may not be able to look at the PR / related code until this weekend

Is it clearer now?

Yes

What happens if you replace if target_window == None with if target_window is None?

@m1guelperez
Copy link
Contributor Author

What happens if you replace if target_window == None with if target_window is None?

That should indeed work! However, I think we should still implement the __eq__ in a different way or?

@m1guelperez
Copy link
Contributor Author

Closed #505.

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