Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

added decimal note/rest lengths #50

Merged
merged 2 commits into from
Sep 28, 2017
Merged

Conversation

pzxwang
Copy link
Contributor

@pzxwang pzxwang commented Sep 25, 2017

For issue #16. I also allowed for decimal rest lengths since they are parsed similarly to note lengths.

Thank you for providing some low-hanging fruit for those of us just starting out.

To follow up, should expressing length in seconds also be allowed in decimals (e.g. 2.5s)? I think millisecond values have to be integers, though.
As for the set-note-length function discussed in the issue, I noticed that in the documentation you suggest just calling the note-length function inside set-duration. Would you still like to add set-note-length as a convenience function of sorts?

Copy link
Member

@daveyarwood daveyarwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! In general I think this looks good, I just have a couple of aesthetic nitpicks.

Re: lengths in seconds, I'd rather not allow those to be expressed with decimals, for the sake of simplicity. We do have millisecond lengths, and I don't think it's too cumbersome to enter 2500ms instead of 2.5s.

I would still like to add a set-note-length convenience function to alda.lisp; I think we could put it in alda.lisp.attributes below where set-duration is defined.

@@ -31,14 +39,20 @@
(alda.lisp/duration (alda.lisp/ms 2000)))]))))

(deftest tie-and-slur-tests
(testing "ties"
(testing "ties and slurs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, I'm not sure why I had it nested like this in the first place:

  (testing "ties"
    (testing "ties"
      ...

Probably a silly error my part. I think we can remove a layer of nesting and just do this:

(deftest tie-and-slur-tests
  (testing "ties"
    ...)
  (testing "slurs"
    ...))

@@ -115,7 +114,7 @@
(= :EOF x)
parser

(#{\newline "\n"} x)
(#{\newline "\n"}x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this was intentional, but could we back this change out, i.e. put the space back in?

@daveyarwood
Copy link
Member

@pzxwang: Looks even better now, tests pass, and it looks like it's working as expected when I test it.

Excellent work, thank you! 👍

@daveyarwood daveyarwood merged commit 8cc82dd into alda-lang:master Sep 28, 2017
@daveyarwood daveyarwood mentioned this pull request Sep 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants