search

mattwparas / steel

An embedded scheme interpreter in Rust
Apache License 2.0
1.07k stars 50 forks source link

update steel/ord #252

Closed fominok closed 4 weeks ago

fominok commented 1 month ago

Hey, I'm not sure what is the goal regarding consistency with Scheme, but according to Racket docs for >, <, >= and <= functions both args shall be real?. Also things like (< 1337 "leet") didn't fail either.

This PR updates steel/ord module for real numbers comparison as well as adds missing documentation.

P.S. I tried to implement something like numeric tower as well to promote types in some more typesafe way to possibly reuse it in other modules, but failed miserably, shall be a separate effort I suppose, going the most direct way now.

fominok commented 1 month ago

Unfortunately I can't run all tests locally to see what's going on, but my not so wild guess is that ord tests exist for strings and for numbers and both use same functions, so fix for strings to use char comparison functions breaks the same test that is fed with numbers; i don't know what the final decision here, shall > be generic as possible or we still take inspiration from Racket?

mattwparas commented 1 month ago

Unfortunately I can't run all tests locally to see what's going on, but my not so wild guess is that ord tests exist for strings and for numbers and both use same functions, so fix for strings to use char comparison functions breaks the same test that is fed with numbers; i don't know what the final decision here, shall > be generic as possible or we still take inspiration from Racket?

Let me run your tests locally and I'll report back what I find

mattwparas commented 1 month ago

To answer the notion that < should only work with numbers, I'm inclined to agree with that sentiment, matching racket and guile would be a good idea then

mattwparas commented 1 month ago

Also, running the tests locally, seems like just a mixup - you did edit the trie.rkt but the test is running a different trie file - just update cogs/sorting/trie-sort.scm to with the char<? and the tests will pass

fominok commented 1 month ago

Also, running the tests locally, seems like just a mixup - you did edit the trie.rkt but the test is running a different trie file - just update cogs/sorting/trie-sort.scm to with the char<? and the tests will pass

it seems there are two trie-sort.scm in tests and I fixed only one besides trie.rkt, thanks

fominok commented 1 month ago

Since it's passing, does it require some extra work to be merged?

mattwparas commented 1 month ago

Sorry! Been a very busy few days. I will review it today