Rethink Chord toList functions so their behavior is more intuitive.

[cianoc]

>>> toList $ scale c dorian
[c]
>>> toList $ chord c minor
[c]

Not sure what the correct behavior should be here, but this feels wrong. I feel it should probably return the generator set:

>>> toList $ scale c dorian
[c, d, eb, f, g, a, bb]
>>> toList $ chord c minor
[c, eb, g]

Obviously this may have repercussions I may not be aware of, which is why I'm raising here, but maybe it would help with this:

-- TODO semantically suspect!
scaleToList :: AffinePair v p => Scale v p -> [p]

and

-- TODO semantically suspect!
chordToList :: AffinePair v p => Chord v p -> [p]

As these could be replaced by toList which feels a little more general and consistent.

I'd be happy to do a PR for this if it makes sense.

[hanshoglund]

Sadly this won't work, as the AffinePair constraint is not compatible with the types of foldMap/toList/traverse. You can however use the existing functions for this:

>>> scaleToList $ scale c dorian
[c, d, eb, f, g, a, bb]

>>> chordToList $ chord c minorTriad 
[c,eb,g]

Thinking over this I regret marking these as "suspect": they are perfectly valid folds, just not compatible with the standard library definitions (which require the traversed type to be fully parametric). Some things that could make sense:

• Remove the "suspect" warning
• Document, test and possible rename the ...toList functions
• Optionally provide traversals.

The last option is a mainly a question on how general we want to go, e.g. compare

generator :: Traversal' (Chord Interval Pitch) Pitch
generator :: AffinePair v p => Traversal' (Chord v p) p
generator :: AffinePair v p => Traversal' (Chord v p) p
generator :: (AffinePair v p, AffinePair v p') => Traversal (ScaleChord o r v p) (ScaleChord o r v p')

[cianoc]

I guess the thing that mostly threw me here was that toList isn't really documented. Maybe if there's an explanation of what it does, maybe with why it could be useful, that could be sufficient. I think also saying that you probably want to use scaleToList, or chordToList instead would be useful.

Renaming it could help, though honestly not really understanding its intended purpose I couldn't say what it should be used for.

[hanshoglund]

Note that toList is from standard library (defined for the Foldableclass). As always when using these very generic classes is that though the behavior of instances might make sense theoretically, it is not always "obvious".

In the User Guide I tend to avoid type-classes in favor of explicit module prefixes, e.g. Voice.map (+ 1) rather than fmap (+ 1). We could do something similar here, e.g. having ScaleChord.toList, Scale.toList, Chord.toList etc. To support this, we would move ScaleChord to its own module, and the provide separate Scale and Chord modules (mostly reexporting).

[cianoc]

Yeah I have mixed feelings about usual type classes the way that Haskell does. It tends to make libraries quite opaque even if you know the typeclasses.

As an aside, Voice.map doesn't work when you use the prelude. So actually I've been using fmap. I actually assumed that the documentation was just lagging the library.

[cianoc]

Just renaming this to reflect the discussion above. I feel a couple of things here are low hanging fruit (e.g. remove suspect comments and documenting what toList actually does in this case).

I don't feel I have enough familiarity with the library yet to comment on the other suggestions.

[hanshoglund]

This also affects the Voicing type.

A straightforward solution is to remove the Foldable/Traversable instances from the types, or move them to newtype wrappers if desired.

The documentation should probably also explain that the recommended way to convert a chord to a list of pitches is to:

1. Voice the chord using one of the functions in Music.Pitch.Voicing.
2. Extract the pitch list with getVoiced.

We could have wrappers for the most common cases in Music.Pitch.Scale, e.g.

toNonEmptyCloseVoicing :: Chord v p -> NonEmpty p
toListCloseVoicing :: Chord v p -> [p]