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
refactor: PositionedNote composition with Note instead of inheritance #113
Conversation
Pull Request Test Coverage Report for Build 4974174985
💛 - Coveralls |
I'm totally fine with renaming it to
I prefer to keep pull requests as straightforward and concise as possible, so I would do it in a separate PR. Please note that I created a PR for this some days ago: #99 (comment). Thanks! |
ee996d0
to
e58af6d
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.
Cool! 💎
@@ -136,15 +138,14 @@ final class PositionedNote extends Note { | |||
|
|||
@override | |||
bool operator ==(Object other) => | |||
super == other && other is PositionedNote && octave == other.octave; | |||
other is PositionedNote && note == other.note && octave == other.octave; | |||
|
|||
@override | |||
int get hashCode => Object.hash(super.hashCode, octave); |
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.
This deprecated hashCode
was left behind, as no exhaustive tests existed to catch a bad implementation there.
Fixed in #155.
Closes #106
Comments:
semitones
doesn't collide withNote.semitones
, I renamedsemitonesFromRootHeight
tosemitones
to conform withMusicItem
, but perhaps you prefer keeping the more explicit name and adding an aliassemitones => semitonesFromRootHeight
?Transposable<T>
definition change in this PR now that it is easier, or in a separate one?