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

feat: add operator.list support for OCaml binding #3706

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

Young-Flash
Copy link
Member

just new to OCaml, appericiate for any review suggestion

cc #2756

@PsiACE
Copy link
Member

PsiACE commented Dec 3, 2023

cc @Ranxy , are you interested in reviewing this PR?

@Ranxy
Copy link
Contributor

Ranxy commented Dec 4, 2023

It seems that the test will fail to run on my machine. After adding the printer, I found the following error

File "test/test.ml", line 97, characters 1-1:
Error: suite:6:test_list (in the code).

expected: [bar; foo] but got: [foo; bar]

It seems that this is a problem with the order of the list results?
I'm not very clear about the specific implementation of the list in opendal rust core, but it seems that the order of the results of the list is different in different platforms, because this test is passed in CI. If so how should the results of the test be sorted?

bindings/ocaml/lib/operator.ml Show resolved Hide resolved
bindings/ocaml/lib/operator.ml Outdated Show resolved Hide resolved
bindings/ocaml/src/operator/entry.rs Outdated Show resolved Hide resolved
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

LGTM. But I'm not familiar with OCaml. Let's wait for @Ranxy's review.

@Young-Flash Young-Flash merged commit 876b185 into apache:main Dec 6, 2023
30 checks passed
@Young-Flash Young-Flash deleted the ocaml_list_support branch December 6, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants