Skip to content

Simple builtin additions to String and Array functions - #1669

Merged
berekuk merged 5 commits into
developfrom
simple-builtin-additions
Apr 6, 2023
Merged

berekuk merged 5 commits into
developfrom
simple-builtin-additions

Conversation

@OAGr

@OAGr OAGr commented Apr 5, 2023

Copy link
Copy Markdown
Contributor

This does a few things:

  1. "foo" + "bar" + normal(3,3)
  2. List.flatten() and List.join()
  3. Forbid non-integer array lookups

I wasn't sure about some of this, happy to accept comments/suggestions.

This closes #1614

Also, this closes the comment to #1327

@OAGr
OAGr requested review from Hazelfire and berekuk as code owners April 5, 2023 04:42
@vercel

vercel Bot commented Apr 5, 2023 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
relative-values ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 6, 2023 7:47pm
squiggle-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 6, 2023 7:47pm
squiggle-website ✅ Ready (Inspect) Visit Preview Apr 6, 2023 7:47pm

@OAGr OAGr changed the title Simple builtin additions Simple builtin additions to String and Array functions Apr 5, 2023
@codecov-commenter

codecov-commenter commented Apr 5, 2023 •

Copy link
Copy Markdown

Codecov Report

Merging #1669 (99ed3c3) into develop (72a77df) will increase coverage by 0.09%.
The diff coverage is n/a.

❗ Current head 99ed3c3 differs from pull request most recent head f86036b. Consider uploading reports for the commit f86036b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop    #1669      +/-   ##
===========================================
+ Coverage    74.00%   74.09%   +0.09%     
===========================================
  Files           93       93              
  Lines         5112     5134      +22     
  Branches       861      865       +4     
===========================================
+ Hits          3783     3804      +21     
- Misses        1304     1305       +1     
  Partials        25       25              

see 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vercel
vercel Bot temporarily deployed to Preview – relative-values April 5, 2023 05:03 Inactive
@OAGr

OAGr commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

Also should close: #843

@NunoSempere

Copy link
Copy Markdown
Contributor

Could we also have List.append? Shouldn't be that different from List.join.

@berekuk

berekuk commented Apr 5, 2023 •

Copy link
Copy Markdown
Collaborator

"foo" + "bar" + normal(3,3)

I'm unsure about this. JS allows this, but Python doesn't ("abc" + 3 -> TypeError: can only concatenate str (not "int") to str), and JS infamously has many problems related to implicit casting.

I don't see anything terrible about this specific feature, though, just worried that having too many implicit casting features like this eventually combine into wats. So it seems safer to implement this only for strings for now.

Same concern about List.join: maybe it should be for lists of strings only, and in other cases we should ask for explicit list -> map(toString) -> join(',').

Note: string interpolation support to Squiggle (`foobar${normal(3,3)}`) is possible but somewhat hard, because of how parsers work (only some parsers are capable of recursively re-entering the parser from inside the string token; Peggy might be able to handle this, but I'm not sure).

@OAGr

OAGr commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

Could we also have List.append? Shouldn't be that different from List.join.

I'll do this in a different PR, is easy enough.

@OAGr

OAGr commented Apr 6, 2023

Copy link
Copy Markdown
Contributor Author

I changed it so that List.join only works on lists of strings.

I think I want to keep the + coercion, especially because I could see it being a while until we have much better methods, like proper string introspection.

get(key: Value) {
if (key.type === "Number") {
const index = key.value | 0; // TODO - fail on non-integer indices?
if (!isInteger(key.value)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wanted to recommend Number.isInteger in place of lodash here, as I often do, but I benchmarked both versions and the performance is the same, so it doesn't matter.

@berekuk
berekuk merged commit dca4273 into develop Apr 6, 2023
@berekuk
berekuk deleted the simple-builtin-additions branch April 6, 2023 21:01

This branch was successfully deployed

2 active and 1 inactive deployments
Preview – squiggle-website — 89b3a347 Deployed Apr 6, 2023 by vercel[bot]
Preview – relative-values — 89b3a347 Deployed Apr 6, 2023 by vercel[bot]
Preview – squiggle-components — 89b3a347 Deployed Apr 6, 2023 by vercel[bot]
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.

Forbid array index lookups for invalid numbers

4 participants