-
Notifications
You must be signed in to change notification settings - Fork 17
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
Rethink Chord toList functions so their behavior is more intuitive. #64
Comments
Sadly this won't work, as the >>> 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:
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') |
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. |
Note that In the User Guide I tend to avoid type-classes in favor of explicit module prefixes, e.g. |
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. |
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. |
This also affects the A straightforward solution is to remove the The documentation should probably also explain that the recommended way to convert a chord to a list of pitches is to:
We could have wrappers for the most common cases in toNonEmptyCloseVoicing :: Chord v p -> NonEmpty p
toListCloseVoicing :: Chord v p -> [p] |
Not sure what the correct behavior should be here, but this feels wrong. I feel it should probably return the generator set:
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:
and
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.
The text was updated successfully, but these errors were encountered: