Check for list emptiness before checking variants

[Vincent-lau]

No description provided.

[mseri]

Why? Filter of an empty list is already the empty list

[Vincent-lau]

sorry meant to do this in the List.fold below. Fixed now

[MarkSymsCtx]

Might be worth adding to the commit message the example failure that raised this

dune build @python --profile=release
File "ocaml/xapi-storage/python/xapi/storage/api/v5/dune", line 1, characters 0-204:
1 | (rule
2 |   (targets datapath.py plugin.py task.py volume.py)
3 |   (alias python)
4 |   (deps
5 |     (:x ../../../../../generator/src/main.exe)
6 |     (source_tree ../../../../)
7 |   )
8 |   (action (run %{x} gen_python -p .))
9 | )
main: internal error, uncaught exception:
      Failure("tl")
      
make: *** [build] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.3pD9wu (%build)

Which resulted from trying to build this change - MarkSymsCtx/xen-api@692d17d

[mseri]

Suggested change

	| variants_to_check ->
	List.fold_left
	(fun acc x -> List.concat [ acc; check false x ])
	(check true (List.hd variants_to_check))
	(List.tl variants_to_check)
	| v :: vs ->
	List.fold_left
	(fun acc x -> List.concat [ acc; check false x ])
	(check true v) vs

You can simplify this further with pattern matching

[mseri]

Would be probably worth to add a test, so that there are no regressions when updating the code in the future

[MarkSymsCtx]

@Vincent-lau I think @mseri meant that you didn't need the | [] -> [] if you used | v :: vs -> form

[mseri]

You need both, the second one pattern matches only on nonempty lists

[Vincent-lau]

unit tests written, should be good to go

[mseri]

If the test requires the use of ppx, please move it to the tests/ppx test suite, this is a bit annoying but avoids circular dependencies when we publish the packages on opam

[MarkSymsCtx]

If the test requires the use of ppx, please move it to the tests/ppx test suite, this is a bit annoying but avoids circular dependencies when we publish the packages on opam

In fact, could it not go into https://github.com/mirage/ocaml-rpc/blob/master/tests/ppx/variants.ml ?

[Vincent-lau]

I think there is a test_pythongen.ml in rpc/ already, so we should probably move it there. And it is already doing @@deriving rpcty