Repository: YatingMusic/miditoolkit

Issue:
`remove_invalid_notes` never removes anything unless `verbose=False`
See
https://github.com/YatingMusic/miditoolkit/blob/38cfa38e7525e9ceb69720e64e2bd49b3eff644a/miditoolkit/midi/containers.py#L287-L305

especially line 300; if `verbose=True`, the code will never reach the actual removal loop.

Discussion:
codecov-commenter: ## [Codecov](https://app.codecov.io/gh/YatingMusic/miditoolkit/pull/42?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
Attention: `1 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`c81dfe2`)](https://app.codecov.io/gh/YatingMusic/miditoolkit/commit/c81dfe2b5fc87ad40b2d5e9a573a503cd7c95c1b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 58.83% compared to head [(`a4270b7`)](https://app.codecov.io/gh/YatingMusic/miditoolkit/pull/42?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 61.53%.

| [Files](https://app.codecov.io/gh/YatingMusic/miditoolkit/pull/42?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Patch % | Lines |
|---|---|---|
| [miditoolkit/midi/containers.py](https://app.codecov.io/gh/YatingMusic/miditoolkit/pull/42?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-bWlkaXRvb2xraXQvbWlkaS9jb250YWluZXJzLnB5) | 87.50% | [1 Missing :warning: ](https://app.codecov.io/gh/YatingMusic/miditoolkit/pull/42?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) |

:exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   58.83%   61.53%   +2.70%     
==========================================
  Files          12       13       +1     
  Lines         770      780      +10     
==========================================
+ Hits          453      480      +27     
+ Misses        317      300      -17     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/YatingMusic/miditoolkit/pull/42?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

Natooz: (fixing what's broken)

Patch:
diff --git a/miditoolkit/midi/containers.py b/miditoolkit/midi/containers.py
index 4e0fe6b..bf78592 100755
--- a/miditoolkit/midi/containers.py
+++ b/miditoolkit/midi/containers.py
@@ -1,4 +1,5 @@
 import re
+import warnings
 from dataclasses import dataclass
 from typing import List, Optional, Union
 
@@ -38,9 +39,9 @@ class Pedal:
     Parameters
     ----------
     start : int
-        Time where the pedal starts.
+        Time when the pedal starts.
     end : int
-        Time where the pedal ends.
+        Time when the pedal ends.
 
     """
 
@@ -61,7 +62,7 @@ class PitchBend:
     pitch : int
         MIDI pitch bend amount, in the range ``[-8192, 8191]``.
     time : int
-        Time where the pitch bend occurs.
+        Time when the pitch bend occurs.
 
     """
 
@@ -80,7 +81,7 @@ class ControlChange:
     value : int
         The value of the control change, in ``[0, 127]``.
     time : int
-        Time where the control change occurs.
+        Time when the control change occurs.
 
     """
 
@@ -284,26 +285,26 @@ class Instrument:
         self.control_changes = [] if control_changes is None else control_changes
         self.pedals = [] if pedals is None else pedals
 
-    def remove_invalid_notes(self, verbose: bool = True):
-        """Removes any notes whose end time is before or at their start time."""
-        # Crete a list of all invalid notes
-        notes_to_delete = [note for note in self.notes if note.end <= note.start]
-        if verbose:
-            if len(notes_to_delete):
-                print("\nInvalid notes:\n", notes_to_delete, "\n\n")  # noqa: T201
-            else:
-                print("no invalid notes found")  # noqa: T201
-            return True
-
-        # Remove the notes found
-        for note in notes_to_delete:
-            self.notes.remove(note)
-        return False
+    def remove_invalid_notes(self, verbose: bool = True) -> None:
+        warnings.warn(
+            "Call remove_notes_with_no_duration() instead.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        return self.remove_notes_with_no_duration()
+
+    def remove_notes_with_no_duration(self) -> None:
+        """Removes (inplace) notes whose end time is before or at their start time."""
+        for i in range(self.num_notes - 1, -1, -1):
+            if self.notes[i].start >= self.notes[i].end:
+                del self.notes[i]
+
+    @property
+    def num_notes(self) -> int:
+        return len(self.notes)
 
     def __repr__(self):
-        return 'Instrument(program={}, is_drum={}, name="{}")'.format(
-            self.program, self.is_drum, self.name.replace('"', r"\"")
-        )
+        return f"Instrument(program={self.program}, is_drum={self.is_drum}, name={self.name}) - {self.num_notes} notes"
 
     def __eq__(self, other):
         # Here we check all tracks attributes except the name.
diff --git a/miditoolkit/midi/parser.py b/miditoolkit/midi/parser.py
index 0377722..e26b275 100755
--- a/miditoolkit/midi/parser.py
+++ b/miditoolkit/midi/parser.py
@@ -38,20 +38,18 @@ class MidiFile:
         charset: str = "latin1",
     ):
         # create empty file
-        if filename is None and file is None:
-            self.ticks_per_beat = ticks_per_beat
-            self.max_tick = 0
-            self.tempo_changes = []
-            self.time_signature_changes = []
-            self.key_signature_changes = []
-            self.lyrics = []
-            self.markers = []
-            self.instruments = []
-
-        # load
-        else:
+        self.ticks_per_beat: int = ticks_per_beat
+        self.max_tick: int = 0
+        self.tempo_changes: Sequence[TempoChange] = []
+        self.time_signature_changes: Sequence[TimeSignature] = []
+        self.key_signature_changes: Sequence[KeySignature] = []
+        self.lyrics: Sequence[str] = []
+        self.markers: Sequence[Marker] = []
+        self.instruments: Sequence[Instrument] = []
+
+        # load file
+        if filename or file:
             if filename:
-                # filename
                 mido_obj = mido.MidiFile(filename=filename, clip=clip, charset=charset)
             else:
                 mido_obj = mido.MidiFile(file=file, clip=clip, charset=charset)
@@ -330,6 +328,10 @@ class MidiFile:
             self.ticks_per_beat, self.max_tick, self.tempo_changes
         )
 
+    @property
+    def num_instruments(self) -> int:
+        return len(self.instruments)
+
     def __repr__(self):
         return self.__str__()
 
@@ -342,7 +344,7 @@ class MidiFile:
             f"key sig: {len(self.key_signature_changes)}",
             f"markers: {len(self.markers)}",
             f"lyrics: {bool(len(self.lyrics))}",
-            f"instruments: {len(self.instruments)}",
+            f"instruments: {self.num_instruments}",
         ]
         output_str = "\n".join(output_list)
         return output_str


Test patch:
diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml
index d619caa..53b9754 100644
--- a/.github/workflows/pytest.yml
+++ b/.github/workflows/pytest.yml
@@ -23,7 +23,7 @@ jobs:
         with:
           python-version: ${{ matrix.python-version }}
           cache: pip
-          cache-dependency-path: setup.py
+          cache-dependency-path: pyproject.toml
       - name: Install dependencies
         run: |
           # Install local package with tests dependencies extras
@@ -36,9 +36,9 @@ jobs:
   build:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - name: Set up Python
-        uses: actions/setup-python@v3
+        uses: actions/setup-python@v4
         with:
           python-version: '3.x'
       - name: Install dependencies
diff --git a/tests/test_utils.py b/tests/test_utils.py
new file mode 100644
index 0000000..1ca2872
--- /dev/null
+++ b/tests/test_utils.py
@@ -0,0 +1,24 @@
+from operator import attrgetter
+
+import pytest
+
+from miditoolkit import MidiFile, Note
+from tests.utils import MIDI_PATHS
+
+
+@pytest.mark.parametrize("midi_path", MIDI_PATHS[:5], ids=attrgetter("name"))
+def test_remove_notes_with_no_duration(midi_path, tmp_path):
+    """Test that a MIDI loaded and saved unchanged is indeed the save as before."""
+    # Load the MIDI file and removes the notes with durations <= 0
+    midi = MidiFile(midi_path)
+    midi.instruments[0].remove_notes_with_no_duration()
+    num_notes_before = midi.instruments[0].num_notes
+
+    # Adding notes with durations <= 0, then reapply the method
+    midi.instruments[0].notes.append(Note(50, 50, 100, 100))
+    midi.instruments[0].notes.append(Note(50, 50, 101, 100))
+    midi.instruments[0].remove_notes_with_no_duration()
+
+    assert (
+        midi.instruments[0].num_notes == num_notes_before
+    ), "The notes with duration <=0 were not removed by test_remove_notes_with_no_duration"