music-notation-swift / music-notation-core

Music Notation Library in Swift (Deprecated. See `music-notation` instead)
MIT License
115 stars 16 forks source link

Note collection index struct #123

Closed massimobio closed 6 years ago

massimobio commented 6 years ago

As per #121 I converted NoteCollectionIndex from a tuple to a struct. (I misspelled tuple as tuplet in the commit message 🤦🏼‍♂️) I noticed that the noteFromMeasure method in MeasureTests could be simplified if we passed a NoteCollectionIndex parameter instead each value separately but the visibility is internal. What would we be the best thing to do?

codecov[bot] commented 6 years ago

Codecov Report

Merging #123 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #123   +/-   ##
=======================================
  Coverage   97.48%   97.48%           
=======================================
  Files          32       32           
  Lines        5809     5809           
=======================================
  Hits         5663     5663           
  Misses        146      146
Impacted Files Coverage Δ
Sources/Measure.swift 88.98% <100%> (ø) :arrow_up:
Tests/MusicNotationCoreTests/MeasureTests.swift 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4cb0327...c42c591. Read the comment docs.

drumnkyle commented 6 years ago

Good thinking that NoteCollectionIndex can be used in the test method as well. Let's make that change here. In Swift, if something is marked internal, you can access it from a test if you use @testable import FrameworkName, which is already being done. Therefore, you should be able to access it. Since it is a struct within a class, you will need to access it by doing Measure.NoteCollectionIndex.

drumnkyle commented 6 years ago

The other places to do this are listed in this doc. I would recommend reading the doc as it will explain more about what this is and why we created it. The bottom of the doc lists the usages. Also, if you have a different or better idea, let us know. This is the pattern we came up.

It'll probably make sense to have separate commits to this PR for each one (you can use the label "WIP" to indicate it's not done yet). Then, we can merge it all at once and close the issue.

massimobio commented 6 years ago

Looks like it's not worth changing that private method in the MeasureTests as it's in use in a lot of places where the syntax would become more verbose.

This latest commit replaces the tuple with struct Staff.MeasureIndex. There are a couple of places where you had used measureIndex for local parameter names so I changed those to be simply index. The keyword index is getting a bit overloaded with different meanings...

If I understand correctly, the Tuplet doesn't use a tuple for indexing so this should be all for this PR.

drumnkyle commented 6 years ago

Once you resolve the conflicts and feel this is ready, you can remove the WIP tag and let me know and I'll review.

massimobio commented 6 years ago

Fixed this. Piece of cake in comparison to the clef stuff...