Skip to content
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

Extend functionality of degree function #2051

Merged
merged 5 commits into from May 17, 2019

Conversation

Projects
None yet
3 participants
@emlyn
Copy link
Contributor

commented Apr 11, 2019

Extend the degree function to:

  • allow degrees larger than one octave, such as a thirteenth
  • allow augmented and diminished degrees by prefixing with an a/d, and doubly augmented/diminished with aa/ dd, as well p for perfect intervals, for completeness.
  • allow numeric strings/symbols as well as roman numerals, so you can use 'd5'/:d5 as well as :dv for diminished 5th

The documentation will also need updating, but I wanted to check if this looks reasonable first.

@emlyn emlyn force-pushed the emlyn:more-degrees branch from 77a749c to 54026f1 Apr 11, 2019

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I realise that from_roman supporting numbers up to >4000 is a slight overkill, but I wasn't sure where to draw the line (maybe only supporting digits i, v & x would be reasonable?). Also it might make sense to move it somewhere more general in case it's useful elsewhere.
There is a slight ambiguity in that d is both a prefix for "diminished", and the number 500, but that's unlikely to be a problem in practise.

@samaaron

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

Looks fab, thanks!

Is there any chance you could update the documentation appropriately? Thanks also for the tests :-)

@samaaron

This comment has been minimized.

Copy link
Owner

commented on 1147451 Apr 24, 2019

Awesome! May I continue to be a pain and ask if you could add some documentation examples? :-)

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

It wasn't obvious to me how to get all the relevant info concisely into the documentation... how does this look?

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Please continue to be a pain :-) I'm happy if it makes this better!

I'll think of a couple of examples and add them later.

@samaaron

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2019

Ah, sorry for not being clearer. This is via a weird doc system I created just for Sonic Pi so there's no wonder it's non-obvious ;-)

There's essentially a big metadata map for each public function and one of the keys of this map is examples: which has a list as a value with each element in the list being a separate example. The funky part is that the comments will be automatically moved to the right hand side when formatting the documentation for viewing, so it's possible to copy the code independently of the descriptive comments.

Here's an example: https://github.com/samaaron/sonic-pi/blob/master/app/server/ruby/lib/sonicpi/lang/sound.rb#L3615

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Thanks for pointing that out, I had noticed the examples before, but forgot. I've removed the "E.g." part from the doc string and added a couple of examples.

I hope they aren't too complicated, I wanted to include an example of using it to create a chord that's not built in to Sonic Pi.

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Just want to check that we're ok with showing the bracket syntax for blocks {...} (eg in the examples here) @samaaron given that it's not officially documented as part of the Sonic Pi DSL? Looks great anyway @emlyn!

@samaaron

This comment has been minimized.

Copy link
Owner

commented Apr 25, 2019

Agree with @ethancrawford, the following doesn't have any direct instruction in the tutorial:

[:i, :iii, :v, :dvii, :dix, :Axi, :xiii].map {|d| (degree d, :Fs, :major)}

Whilst it's valid ruby, it's unsupported Sonic Pi code as nowhere do we cover mapping functions over lists.

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Yes, that's a fair point. Is there a supported way of doing something similar? What about:

[:i, :iii, :v, :dvii, :dix, :Axi, :xiii].each do |d|
  play (degree d, :Fs, :major)
end

although it's not quite the same, as it won't scale the volume according to the number of notes.
I could always remove the loop entirely:

play [(degree :i, :Fs, :major), (degree :iii, :Fs, :major), (degree :v, :Fs, :major),
      (degree :dvii, :Fs, :major), (degree :dix, :Fs, :major), (degree :Axi, :Fs, :major),
      (degree :xiii, :Fs, :major)]

but I think that's a bit too verbose/repetitive.

@emlyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

How about this, would it be acceptable? Or else do you have any suggestions on how to improve it?

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

🤔 hmmm
Well, technically speaking, .each is only ever used a few times in 'documentation', and I think only in the example compositions - not in the tutorial or references per se. append on the other hand is pure ruby that is definitely not used anywhere user facing at all as far as I can tell. (Though I do see that you have added comments to try to mitigate these facts).
As for a better solution, I'm not 100% sure. Any ideas @samaaron ?

@samaaron samaaron merged commit 774fbd3 into samaaron:master May 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@samaaron

This comment has been minimized.

Copy link
Owner

commented May 17, 2019

Thank-you :-)

@ethancrawford

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@samaaron I take it you were ok with the pure ruby functions in this PR, as described in my comment above?

@samaaron

This comment has been minimized.

Copy link
Owner

commented May 17, 2019

@ethancrawford I might still tweak them before release, but the PR is too good to not pull in :-)

@emlyn emlyn deleted the emlyn:more-degrees branch May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.