Conversation
|
Ah, I guess this still isn't working, I think because I'm still relying on some type inference that GHCi adds. I'll keep working on it. |
|
(sorry marked as 'ready to review' with a stray click..) |
|
Can this be merged? (besides the conflicts) |
|
Almost—some bug fix I made broke the overloaded single ID/list of IDs thing. But since that was always kind of a fun extra feature, I think I'm going to abandon it for now, and just include the working single ID type. I'll push an update shortly that includes just that fix and resolves all conflicts. |
matthewkaney
force-pushed
the
pattern-ids
branch
from
9781d2e to
d577f52
Compare
|
Okay, @yaxu, the simplified version of this change is ready for review! Let me know if there's anything here that you want changed. On compatibility, this is technically a breaking change for anyone who's used Fixes #807. Also, maybe #846, but I'm not sure if there's anything specific to tidal-listener that's still captured by that issue. |
src/Sound/Tidal/ID.hs
Outdated
| -- | Typeclass for things that can be coerced to a string and used as an identifier. | ||
| -- | Similar to Show, but contrained to strings and integers and designed so that | ||
| -- | similar cases (such as 1 and "1") refer to the same value. | ||
| newtype ID = ID { toID :: String } |
There was a problem hiding this comment.
maybe fromID would be more correct, since it gives the String value of the ID?
There was a problem hiding this comment.
The Stream module refers to strings as "pattern ids", so I think that's where I was coming from, but I agree it's confusing with an explicit ID type.
After reading a bit into naming conventions, I think I like unID, which is already also the convention that the Note type uses. I went ahead and made that change if that works for you!
(Also pinging @yaxu if he has time to look over this soon—thanks!)
There was a problem hiding this comment.
I think the unNote was an idea of mine, looking at that now is not that good :D
Yes we could follow that convention at first, but also think about to find out which convention is more appreciated by the contributors
There was a problem hiding this comment.
Yes I find unNote a little bit confusing, but don't have strong feelings..
There was a problem hiding this comment.
The un prefix seems to be popular Haskell style for this type of thing, but the Tidal audience isn’t necessarily super Haskell-experienced anyway.
I do like fromID too if you’d rather. I could also change it to fromNote if we wanted them to be consistent and it wouldn’t break things.
Alex, any other things you want me to address on this?
There was a problem hiding this comment.
Regardin unNote, renaming that would be a breaking change, maybe it will be better to do it in another pull request
There was a problem hiding this comment.
unNode / unID is probably fine then. Maybe my confusion is just with the default Show instance
sound "bd" # note 8
(0>1)|note: Note {unNote = 8.0}n, s: "bd"
We could just make a better Show instance then.
There was a problem hiding this comment.
fromNote / fromID also fine if you prefer..
There was a problem hiding this comment.
It sounds like fromID might make the most sense, so I went ahead and changed it to that. I've also noticed the default Show instance for Note—I can put together a pull request to address that, and questions about the naming of unNote can wait for further thought/a major release.
Is this PR ready to merge then?
|
Thanks @mindofmatthew ! |
3 participants
Here's my proposed modification to pattern IDs in line with #803. This would specify that patterns can only be identified by an integer or a string (this could be expanded to include other types that would be useful IDs, but ints and strings were the ones I was aware of being used in practice).
This approach has two benefits:
This now allows for overloaded implementations of[EDIT: This part still has bugs, so I'm dropping it for now]muteetc, that can take either a single ID or a list of IDs for muting multiple channels.mute 1mute "bass"mute [2..6]andmute ["drum", "bass"]are now all legal, and the OSC mirror this behavior by accepting multiple arguments.I was hoping there was some sort of clever abstract typeclassing I could do to prevent the ID module from being so verbose, but I couldn't come up with an approach that didn't create some ambiguity that GHC couldn't resolve. Happy to hear if you've got thoughts, or if you'd prefer that I do anything different with the code in terms of naming, formatting, etc.