feat: implement list->indexOf() #1765

Melkor333 · 2023-12-12T07:09:41Z

As discussed in Zulip :)

andychu · 2023-12-12T21:17:43Z

Ah OK looks like there is a type error

http://travis-ci.oilshell.org/github-jobs/5588/dev-minimal.wwz/_tmp/soil/logs/oil-types.txt

Fixing that may clear up these other errors - http://travis-ci.oilshell.org/github-jobs/5588/#job-dev-minimal

I try to put all the commands here, including type checking, but let us know on Zulip if you have problems getting it green:

https://github.com/oilshell/oil/wiki/Oil-Dev-Cheat-Sheet

Melkor333 · 2023-12-13T10:25:42Z

I really need to get the dev build locally, or I'm gonna abuse the CI hard 😅
FWIW:

sam@klara:~/git/oil$ build/cpp.sh all
build/cpp.sh: line 40: all: command not found

The docs at https://github.com/oilshell/oil/wiki/Oil-Dev-Cheat-Sheet#oil-native-c might need a bit of an update

Edit:

also from https://github.com/oilshell/oil/wiki/Contributing#full-python-build-buildpysh-all:

sam@klara:~/git/oil$ deps/from-tar.sh download-re2c
deps/from-tar.sh: line 90: download-re2c: command not found

Is there one "do all steps" command, or would it make sense to add one to get started a bit more easily?
AFAIU it's something like the following now:

deps/from-tar.sh configure-python
deps/from-tar.sh build-python
deps/from-tar.sh layer-cpython
...

Melkor333 · 2023-12-13T10:41:36Z

Oh I found a comment now in build/deps.sh... Anyway some more helpful input, I think I ran every of the suggested commands and still get the following error:

sam@klara:~/git/oil$ build/py.sh all

...

  CC benchmarks/time-helper.c
Traceback (most recent call last):
  File "/home/sam/git/oil/bin/oils_for_unix.py", line 195, in <module>
    sys.exit(main(sys.argv))
  File "/home/sam/git/oil/bin/oils_for_unix.py", line 170, in main
    return AppBundleMain(argv)
  File "/home/sam/git/oil/bin/oils_for_unix.py", line 151, in AppBundleMain
    return readlink.main(main_argv)
  File "/home/sam/git/oil/tools/readlink.py", line 23, in main
    if not arg.f:
AttributeError: '_Attributes' object has no attribute 'f'

andychu · 2023-12-13T15:26:07Z

Hm sorry about the outdated commands, I fixed it to say build/py.sh all before building C++

https://github.com/oilshell/oil/wiki/Oil-Dev-Cheat-Sheet

Let me fix the re2c thing too

I restarted the flaky CI task and it worked (that is either Github or Dreamhost, not us !)

andychu · 2023-12-13T15:31:20Z

Yeah sorry this page was out-dated

https://github.com/oilshell/oil/wiki/Contributing#full-python-build-buildpysh-all

It's now

build/deps.sh fetch
build/deps.sh install-wedges

But since the CI passes you don't necessarily need to do that. I want people to concentrate on the Python parts first

Though I guess it can be annoying if you see a whole bunch of failing stuff and don't know why

Melkor333 · 2023-12-13T15:35:46Z

FWIW I read the travis logs to see that it was the range and try which caused the issue… I just don't like having to go a full CI-circle every time I change something.

I'm also interested in the C++ build as I'd like to look into packaging at some point. :)

should I squash the commits into one commit?

Oh and thanks a lot for updating the docs right away!

andychu

This looks great, thank you !!

andychu · 2023-12-13T15:32:52Z

spec/ysh-methods.test.sh

@@ -99,6 +99,20 @@ setvar is_a_val = "xyz"
 (Bool)   True
 ## END

+#### Lis->indexOf


typo List

I guess we should write it List => indexOf() because it's non-mutating

I have this idea that -> should be mutating and I/O, and => should be "transformations" or "accessors"

I'm interested in feedback on that

I know that it's unfamiliar, but I think Python and JS are too loose with the distinctions

This is very confusing:

mystr.strip() # does nothing, it should be mystr = mystr.strip() mylist.sort()

So for us that would be

mystr => strip() # looks wrong because => is a transformation setvar mystr = mystr => strip() # right way call mylist->sort()

So you get a little hint with -> or =>

https://oilshell.zulipchat.com/#narrow/stream/384942-language-design/topic/mutation.20vs.2E.20transformation

That makes total sense!
x->transform vs x => pure_func. it also reminds me a bit of the weird haskell chains (though IIRC this goes the other way).

So instead of a method this should become a function then. Let me see what I can do!

nvm it's just syntactic sugar. I genuinly thought => is for differently implemented funcs

Oh yes it's just sugar!

So yes you don't have to change the code at all, just the tests.

I'm very happy for this feedback :) :) since it confirms my hunch

but that means something like x => strip => split would technically work but probably give an unexpected result? or is the idea to enforce one or the other at some point?

x => strip() => split() should work like you expect? what would be unexpected?

andychu · 2023-12-13T15:36:08Z

builtin/method_list.py

+        rd.Done()
+        i = 0
+        while i < len(li):
+            if needle.tag() == li[i].tag() and val_ops.ExactlyEqual(li[i], needle, loc.Missing):


I'm pretty sure you don't need to check the tag, because ExactlyEqual will check it

Tests should still pass

fair thing. in my tired mind that somehow should've been the replacement for the try/catch but it doesn't make sense.

andychu · 2023-12-13T15:39:47Z

FWIW I read the travis logs to see that it was the range and try which caused the issue… I just don't like having to go a full CI-circle every time I change something.

Yes that can be an issue. So I guess I would say try the new instructions

build/deps.sh fetch; build/deps.sh install-wedges

I switched to this "wedges" idea back in April, but I got stuck on root vs. non-root

I wanted to make it run without root, but as it is you will get stuff in /wedge which means you need root

https://github.com/oilshell/oil/wiki/Oil-Dev-Cheat-Sheet says you can rm-oils-crap -- we ONLY put stuff in /wedge and ~/wedge -- that's the idea. So those are the only two dirs you need to know about to clean it up.

I'm also interested in the C++ build as I'd like to look into packaging at some point. :)

The packaging should generally be done from the official tarballs.

It's definitely time to start packaging it soon, though I think JSON support needs to be there for it to work

should I squash the commits into one commit?

No I squash on merge!

Oh and thanks a lot for updating the docs right away!

I should have done that quite awhile ago !! Someone else ran into this :-( Too many docs to juggle!

Melkor333 · 2023-12-13T15:51:53Z

damn i just saw that my commit messages are wrong and squashed anyway :-|

andychu

Changes look good!

andychu · 2023-12-14T22:14:55Z

builtin/method_list.py

@@ -92,7 +92,7 @@ def Call(self, rd):
        rd.Done()
        i = 0
        while i < len(li):
-            if needle.tag() == li[i].tag() and val_ops.ExactlyEqual(li[i], needle, loc.Missing):
+            if val_ops.ExactlyEqual(li[i], needle, loc.Missing):


This looks good, can you try this:

$ bin/ysh -c '= [1.0, 2.0] => indexOf(3.14)' = [1.0, 2.0] => indexOf(3.14) ^ [ -c flag ]:1: fatal: Equality isn't defined on Float

And then try changing loc.Missing to rd.LeftParenToken()

I think it will improve the error

And perhaps add this test case to test/ysh-runtime-errors.sh

It could be in a new function at the end or some existing function

That is how we "eyeball" all the errors to make sure they are decent

I was wondering which var we (currently?) can't compare for equality.

And thanks, I didn't see any other code getting a location from rd so that's why I went with Missing. Definitely better with a proper location!

I added the error to the existing test-fat-arrow func but maybe it's better to have an additional func test-ysh-methods? But I guess we can change this if more methods require a testcase?

andychu · 2023-12-16T00:45:24Z

doc/ref/chap-type-method.md

-### find()
+### indexOf()
+
+Returns -1 if element is not in the index.


How about

"Returns the first index of the element in the List, or -1 if it's not present."

"element is not in the index" is confusing

Sorry I didn't catch this the first time around

should've been "element is not in the List" but your proposal is probably a bit more clear :)

andychu · 2023-12-17T16:18:22Z

Great thank you!

andychu reviewed Dec 13, 2023

View reviewed changes

[builtin/indexOf] Add List method indexOf

08f8795

Melkor333 force-pushed the ysh-list-indexof branch from fb9927c to 08f8795 Compare December 13, 2023 15:50

[builtins/method] simpler if and replace pure -> with => in method_list

de9a112

andychu reviewed Dec 14, 2023

View reviewed changes

Melkor333 added 2 commits December 15, 2023 07:17

use proper location & add err test

e907d77

remove unnecessary import

41d2595

andychu reviewed Dec 16, 2023

View reviewed changes

proper doc for indexOf

0bcfe37

andychu changed the base branch from master to soil-staging December 17, 2023 15:47

Merge branch 'soil-staging' into ysh-list-indexof

c0288b9

andychu merged commit 092479b into soil-staging Dec 17, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement list->indexOf() #1765

feat: implement list->indexOf() #1765

Melkor333 commented Dec 12, 2023

andychu commented Dec 12, 2023

Melkor333 commented Dec 13, 2023 •

edited

Loading

Melkor333 commented Dec 13, 2023

andychu commented Dec 13, 2023

andychu commented Dec 13, 2023

Melkor333 commented Dec 13, 2023 •

edited

Loading

andychu left a comment

andychu Dec 13, 2023

Melkor333 Dec 13, 2023

Melkor333 Dec 13, 2023

andychu Dec 13, 2023

Melkor333 Dec 13, 2023

andychu Dec 13, 2023

andychu Dec 13, 2023

Melkor333 Dec 13, 2023

andychu commented Dec 13, 2023

Melkor333 commented Dec 13, 2023

andychu left a comment

andychu Dec 14, 2023

Melkor333 Dec 14, 2023

Melkor333 Dec 15, 2023

andychu Dec 16, 2023

Melkor333 Dec 17, 2023

andychu commented Dec 17, 2023

feat: implement list->indexOf() #1765

feat: implement list->indexOf() #1765

Conversation

Melkor333 commented Dec 12, 2023

andychu commented Dec 12, 2023

Melkor333 commented Dec 13, 2023 • edited Loading

Melkor333 commented Dec 13, 2023

andychu commented Dec 13, 2023

andychu commented Dec 13, 2023

Melkor333 commented Dec 13, 2023 • edited Loading

andychu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andychu commented Dec 13, 2023

Melkor333 commented Dec 13, 2023

andychu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andychu commented Dec 17, 2023

Melkor333 commented Dec 13, 2023 •

edited

Loading

Melkor333 commented Dec 13, 2023 •

edited

Loading