Conversation
…nstances for new chordal types
…m or Aug get args that aren't > 0 and < 12, changed interval calculations to match
…oIntervals function so that it easily allows for that
… -- the change proposed wasn't unambiguously better and creates a lot of work
|
Sorry I'm so late getting to this; I'll try to do some review tonight, I've been swamped with work stuff / had a bit of an emergency this past weekend. |
ChrisEPhifer
left a comment
ChrisEPhifer
left a comment
There was a problem hiding this comment.
Overall there's a lot of good stuff here :) I like some of the function improvements, they're certainly more idiomatic.
My main complaint is the functions that have types like Maybe Quality -> Maybe Quality; it's a bit odd to see a type like this in the wild. Infinitely more useful, I think, would be functions that look like Quality -> Maybe Quality: These can be used very naturally in monadic contexts, and are built to indicate "I'm sure the thing I'm given is well-formed, but I might be producing an ill-formed result." I'll push some changes that take what you've done here closer to what I think would be idiomatic.
…lity`, updated code dependent on this
ChrisEPhifer
left a comment
ChrisEPhifer
left a comment
There was a problem hiding this comment.
OK, I left some comments on the changes I made so far; there's a little bit more to do, then you can do a review and knock off whatever else you wanted to get done for this PR, and then I'll do one more review before we merge into develop :)
| instance Show ChordShape where | ||
| show c = show (getQuality c) ++ show (getHighestNatural c) | ||
| ++ concat (show <$> getExtensions c) ++ show (getSus c) |
There was a problem hiding this comment.
I simplified this per the suggestion I made to your original commits.
src/Base/Chord/Subs.hs
Outdated
| import Base.Core.Note | ||
| import Base.Core.Quality.CQuality as CQ | ||
| import Base.Core.Quality.IQuality as IQ | ||
| import Base.Core.Quality.IQuality as IQ hiding (major) |
There was a problem hiding this comment.
This was necessary since I added true smart constructors for the Quality type in this module, that is, functions correspondent to each of the constructors for the type. For the time being, the 'real' constructors are still exposed, but I'll probably commit that fix before we merge this in.
src/Base/Core/Quality/IQuality.hs
Outdated
| major :: Quality | ||
| major = Major | ||
|
|
||
| perfect :: Quality | ||
| perfect = Perfect | ||
|
|
||
| minor :: Quality | ||
| minor = Minor | ||
|
|
||
| diminished :: Int -> Maybe Quality | ||
| diminished x = if x > 0 && x < 12 then Just $ Diminished x else Nothing | ||
|
|
||
| augmented :: Int -> Maybe Quality | ||
| augmented x = if x > 0 && x < 12 then Just $ Augmented x else Nothing |
There was a problem hiding this comment.
So when I say "smart constructors", for a sum type like Quality, this is what I mean: One function per constructor in the data declaration that enforces invariants of that constructor. Your original smart constructor, iQualFrom, would have been appropriate if this was a product type, such as any of the various record types we've introduced so far; since it's a type with variants, you need at least one function per variant.
| raisePerfect :: Quality -> Maybe Quality | ||
| raisePerfect Perfect = Just $ Augmented 1 | ||
| raisePerfect (Augmented x) = augmented $ x + 1 | ||
| raisePerfect (Diminished 1) = Just Perfect | ||
| raisePerfect (Diminished x) = diminished $ x - 1 | ||
|
|
||
| -- | Given an interval quality, this raises that quality by a semitone | ||
| -- assuming that its base quality is Major. | ||
| raiseMajor :: Quality -> Quality | ||
| raiseMajor Major = Augmented 1 | ||
| raiseMajor (Augmented x) = Augmented $ x + 1 | ||
| raiseMajor Minor = Major | ||
| raiseMajor (Diminished 1) = Minor | ||
| raiseMajor (Diminished x) = Diminished $ x - 1 | ||
| raiseMajor :: Quality -> Maybe Quality | ||
| raiseMajor Major = Just $ Augmented 1 | ||
| raiseMajor (Augmented x) = augmented $ x + 1 | ||
| raiseMajor Minor = Just Major | ||
| raiseMajor (Diminished 1) = Just Minor | ||
| raiseMajor (Diminished x) = diminished $ x - 1 | ||
|
|
||
| -- | Given an interval quality, this lowers that quality by a semitone | ||
| -- assuming that its base quality is Perfect. | ||
| lowerPerfect :: Quality -> Quality | ||
| lowerPerfect Perfect = Diminished 1 | ||
| lowerPerfect (Diminished x) = Diminished $ x + 1 | ||
| lowerPerfect (Augmented 1) = Perfect | ||
| lowerPerfect (Augmented x) = Augmented $ x-1 | ||
| lowerPerfect :: Quality -> Maybe Quality | ||
| lowerPerfect Perfect = Just $ Diminished 1 | ||
| lowerPerfect (Diminished x) = diminished $ x + 1 | ||
| lowerPerfect (Augmented 1) = Just Perfect | ||
| lowerPerfect (Augmented x) = augmented $ x-1 | ||
|
|
||
| -- | Given an interval quality, this lowers that quality by a semitone | ||
| -- assuming that its base quality is Major. | ||
| lowerMajor :: Quality -> Quality | ||
| lowerMajor Major = Minor | ||
| lowerMajor Minor = Diminished 1 | ||
| lowerMajor (Diminished x) = Diminished $ x + 1 | ||
| lowerMajor (Augmented 1) = Major | ||
| lowerMajor (Augmented x) = Augmented $ x - 1 | ||
| lowerMajor :: Quality -> Maybe Quality | ||
| lowerMajor Major = Just Minor | ||
| lowerMajor Minor = Just $ Diminished 1 | ||
| lowerMajor (Diminished x) = diminished $ x + 1 | ||
| lowerMajor (Augmented 1) = Just Major | ||
| lowerMajor (Augmented x) = augmented $ x - 1 No newline at end of file |
There was a problem hiding this comment.
Quite a bit cleaner, no? :)
|
Grr. Well, I've got some irritating news: I'm not sure there's a great way to fully protect the The way I see it, our two options are:
I'm slightly leaning towards option 1, since we've been really good about protecting invariants just about everywhere else - it's a bit annoying for us, but results in what I think is a more robust API. I'm also sort of considering end-users here, and it's a little annoying for them, too, if they're using these libraries and want to pattern-match. I'll try out the predicates approach, and if it's kinda bad, we can always roll back that/those commit(s). |
So I've added the predicates, but updating the code using What I'd really like here is the ability to expose the representation to only other modules in this library, but prevent end-users from seeing those representations. I'll do some digging to find out if there's anything like that we can do. |
|
Aha! I think I found exactly what we want: There's a language extension called |
|
Also just found |
ChrisEPhifer
left a comment
ChrisEPhifer
left a comment
There was a problem hiding this comment.
OK! I've solved the smart constructor problem with the language extension PatternSynonyms; left some comments for you to check out, let me know if you have questions about this.
Definitely going to think about whether we can use this elsewhere to make our smart constructors nicer; I have a feeling the answer to that is 'yes', but need to spend some more time on it to be sure. I'm hesitant to go too crazy with language extensions for now, if only to protect portability. For what it's worth, though, so far we've only used two extensions, and they're both fairly ubiquitous in Haskell code (the second being FlexibleInstances, which let me instantiate Invertible for every functorial type in one go.)
| case iQual of | ||
| Major -> Minor | ||
| Minor -> Major | ||
| Perfect -> Perfect | ||
| (Augmented x) -> Diminished x | ||
| (Diminished x) -> Augmented x | ||
| Major -> minor | ||
| Minor -> major | ||
| Perfect -> perfect | ||
| (Augmented x) -> fromJust $ diminished x | ||
| (Diminished x) -> fromJust $ augmented x |
There was a problem hiding this comment.
Check it out! We can pattern match using the old constructor names, but we can't actually use those constructors to make new values of IQuality! :) Exactly what we were looking for.
| , pattern Major | ||
| , pattern Minor | ||
| , pattern Perfect | ||
| , pattern Diminished | ||
| , pattern Augmented |
There was a problem hiding this comment.
These exports expose only pattern forms of the constructors for this type; this way, we and other users can pattern-match in a natural way, but can't use the underlying representation. Note that I removed (..) from the export of Quality; this keeps all the actual constructors hidden.
| data Quality | ||
| = Major | ||
| | Perfect | ||
| | Minor | ||
| | Diminished Int | ||
| | Augmented Int | ||
| = RealMajor | ||
| | RealPerfect | ||
| | RealMinor | ||
| | RealDiminished Int | ||
| | RealAugmented Int | ||
|
|
||
| pattern Major <- RealMajor | ||
| pattern Perfect <- RealPerfect | ||
| pattern Minor <- RealMinor | ||
| pattern Diminished x <- RealDiminished x | ||
| pattern Augmented x <- RealAugmented x |
There was a problem hiding this comment.
Here's the magic: I renamed all the actual constructors, and made unidirectional (meaning only usable as patterns) pattern synonyms that have the original names.
2 participants
I knocked down all the TODO's that were littered throughout the repo except for 2, adding smart constructors and adjusting downstream code accordingly. All the changes were fairly local.
Highlights
(I know I've neglected to add docs for a couple straightforward utility functions, sorry)
Things Left as TODO