tmux-python / libtmux

⚙️ Python API / wrapper for tmux
https://libtmux.git-pull.com
MIT License
1.02k stars 104 forks source link

Fixed __eq__ for windows. #505

Closed m1guelperez closed 1 year ago

m1guelperez commented 1 year ago
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"
        )

This code failed before at if target_window == None because comparison for windows was implemented like that:

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

However, the assert seems a bit off since it is a way to strong assumption. Thus I changed it to:

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

This code now properly returns False when we compare a window with == and the condition is indeed False.

JThoennes commented 1 year ago

Actually, I would try to avoid negations with not and express the code like this:

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

Just my two cents.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

codecov[bot] commented 1 year ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4d51e53) 88.84% compared to head (bdb105f) 88.79%.

Files Patch % Lines
src/libtmux/window.py 33.33% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #505 +/- ## ========================================== - Coverage 88.84% 88.79% -0.06% ========================================== Files 35 35 Lines 3514 3515 +1 Branches 484 485 +1 ========================================== - Hits 3122 3121 -1 - Misses 284 285 +1 - Partials 108 109 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tony commented 1 year ago

@m1guelperez Rebased

@JThoennes That makes sense, I will make that change.

tony commented 1 year ago

@m1guelperez Can you sign the CLA?

m1guelperez commented 1 year ago

@m1guelperez Can you sign the CLA?

Done :)

tony commented 1 year ago

@m1guelperez Merged

Thank you both!

tony commented 1 year ago

This is live in v0.25.0