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

fixed: Parser errors for new rest symbol #1091 #1113

Merged
merged 5 commits into from
Jan 24, 2025

Conversation

sss-create
Copy link
Collaborator

as described in #1091

the 'edge cases' resolved were:

  1. the '-' could be a negative number or part of a rational number.
  2. the angles and brackets tokenization from native Parsec behave weird.

puh! this was a tough one for me to find because of point 2.

Tests work, so if @yaxu gives his ok, this can be merged.

@sss-create sss-create requested a review from yaxu January 23, 2025 07:39
@sss-create sss-create linked an issue Jan 23, 2025 that may be closed by this pull request
@yaxu
Copy link
Member

yaxu commented Jan 23, 2025

I'm happy for it to be merged, but perhaps worth adding a test or two to test/Sound/Tidal/ParseTest.hs if you have time?

@sss-create
Copy link
Collaborator Author

sss-create commented Jan 23, 2025

I did. Interesting:
comparing this with itself leads to a failing test. Do I correctly use 3%4 here?
("[~~ 2 <~~ 2@7 3> 3%4 9|8 ~~ [~~ <2 9q> ~]] 2!4" :: Pattern String)

Comparing and using rationals like in the added test passes it.
edit: Checking on the current branch makes it look like this was not introduced by my changes.

@yaxu
Copy link
Member

yaxu commented Jan 23, 2025

Hmm, well values 9q and 3%4 look like they are expecting the parser for Pattern Rational which is parsed differently to Pattern String.

So I would try changing Pattern String for Pattern Rational and see if the tests pass then.

But still, although parsing this as Pattern String would lead to difficult to anticipate results, those results should at least be deterministic, so don't know what's going on with that test..

@sss-create
Copy link
Collaborator Author

Ah yes! On both branches — with and without my changes — the test using Pattern String does fail, while the test parsed as Pattern Rational passes.

@yaxu
Copy link
Member

yaxu commented Jan 23, 2025

Hmm, if I check out your dev branch, the tests pass, and this comparison succeeds:

(queryArc ("[-- 2 <-- 2@7 3> 1*4%2 3? 4 9|8 -- [-- <2 9q> -]] 2!4" :: Pattern String) (Arc 0 127)) == (queryArc ("[~~ 2 <~~ 2@7 3> 1*4%2 3? 4 9|8 ~~ [~~ <2 9q> ~]] 2!4" :: Pattern String) (Arc 0 127))

@yaxu
Copy link
Member

yaxu commented Jan 23, 2025

If I run that on the main tidal dev branch, I get a syntax error from parsec, which I guess would explain the failing test?

@sss-create
Copy link
Collaborator Author

Yes, 1*3%4 works as Pattern String but 3%4 did work as Pattern Rational only. But I am not sure if there is a problem now or everything works, where it should.

@yaxu
Copy link
Member

yaxu commented Jan 23, 2025

I think it's probably working fine!

@sss-create
Copy link
Collaborator Author

I added a test for Pattern Rational. Will merge this now :)

@sss-create sss-create merged commit c885e23 into tidalcycles:dev Jan 24, 2025
19 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parser errors for new rest symbol
2 participants