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

Add annotation for table-from-raw-array(), checking arg is indeed a raw array of rows #1759

Open
wants to merge 3 commits into
base: horizon
Choose a base branch
from

Conversation

ds26gte
Copy link
Contributor

@ds26gte ds26gte commented Sep 9, 2024

addresses issue #1758

@jpolitz
Copy link
Member

jpolitz commented Sep 9, 2024

Thanks!

A few tests here would help make it clear what this is for (and verify the right thing is happening automatically): https://github.com/brownplt/pyret-lang/blob/horizon/tests/pyret/tests/test-tables.arr#L653

You can see some similar style of tests (for dynamic annotation-checking errors in the table library) here: https://github.com/brownplt/pyret-lang/blob/horizon/tests/pyret/tests/test-tables.arr#L507

@ds26gte
Copy link
Contributor Author

ds26gte commented Sep 10, 2024

Just one test added. Is there more variety needed in this situation?

@blerner
Copy link
Member

blerner commented Sep 10, 2024

This looks like it merely tests that the argument is a raw array; it doesn't check that the elements are checked as well.

@ds26gte
Copy link
Contributor Author

ds26gte commented Sep 10, 2024

@blerner, are you referring to having a test case that checks that something like

  [table-from-rows:
    [raw-row: 5, {"B"; 7}, {"C"; 8}],
    [raw-row: {"A"; 1}, {"B"; 2}, {"C"; 3}]]

fails?

Or are you saying that the predicate I used in tables.arr is merely checking that the argument is a raw-array of raw-rows, but doesn't further check that each raw-row contains only key-value pairs? The latter is already taken care of by the existing code once the initial check that it's all raw-rows passes.

I can add the test case above to test-tables.arr, but you already have in place a more stringent test, i.e., that the key-value pairs have matching keys in the correct order across all the rows. This new test is pretty basic in comparison.

@blerner
Copy link
Member

blerner commented Sep 10, 2024

I'm saying you have no test case that fails in the is-row check of https://github.com/brownplt/pyret-lang/pull/1759/files#diff-9e511711584173e90288d8fe12034bea103c3de800fdd9b50fcf38dd0479b273R92. So [table-from-rows: 1, [raw-row: ...], false, [raw-row: ...], "a string that's definitely not a row", ...].

Additionally, implementing the annotation in Pyret as a refinement will give you a lousier error message than if you implemented it as an annotation in JS, as we do in the image library, as I pointed you towards in the original issue.

- a (second) test that checks all values are raw-rows
- one that checks that the raw-rows contain only tuples
@ds26gte
Copy link
Contributor Author

ds26gte commented Sep 10, 2024

Shriram's original example (the one that used a [list: ...] around the [raw-rows: ...]s) -- which I'd already added to the tests actually covers this case. But I've added another, more explicit, example that matches yours.

My question had however to do with the other type of error, where the table-from-rows arguments are indeed raw-rows, but the content of the rows are not tuples, let alone tuples with matching keys. If I'm reading you correctly, you don't want this be tested by the type annotation of table-from-raw-array. It will eventually be caught by the %(is-kv-pairs) or :: KeyValPair<C?>.

I looked at the annListColor defined and used in CPO's internal-image-typed.js. It is used internally within the JS, not exported to be used in an .arr file. If I'm matching that behavior for pyret-lang's tables, I could define a predicate and export it as a method of a table-row (similar to get-column-names()) but this seems wasteful. Wouldn't it make sense to use such a predicate immediately within tables.js (not tables.arr), viz., inside the body of makeRowFromArray(), which doesn't do much checking. (I do see @jpolitz's comment suggesting tests should not be done here though.)

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.

3 participants