ocamltest: refactor run_test_tree#13316
Merged
gasche merged 4 commits intoocaml:trunkfrom Oct 1, 2024
Merged
Conversation
MisterDA
reviewed
Jul 19, 2024
Comment on lines
+154
to
+156
| List.fold_left join_summaries summ | ||
| (List.map (run_tree behavior env All_skipped) subs) |
Contributor
There was a problem hiding this comment.
A candidate for List.fold_left_map?
Member
Author
There was a problem hiding this comment.
Meh, fold_left_map returns a list of results that we don't need, and it expects the function to return a pair which we don't do, so I think the result would be less readable than the current approach.
ocamltest/ocamltest_stdlib.ml
Outdated
Comment on lines
48
to
50
| let rec concatmap f = function | ||
| | [] -> [] | ||
| | x::xs -> (f x) @ (concatmap f xs) |
Contributor
There was a problem hiding this comment.
This exists now, it's List.concat_map.
Member
Author
|
@MisterDA if you want to ask me for more refactoring work than what I need, you need to pay me with a full review :-) |
Member
Author
|
@MisterDA volunteered to review this. |
Closed
MisterDA
approved these changes
Sep 6, 2024
e801fcc to
51f6364
Compare
Octachron
approved these changes
Oct 1, 2024
Member
Octachron
left a comment
There was a problem hiding this comment.
I am ok with decoupling the statement evaluation from the traversal logic.
Suggested-by: Antonin Décimo <antonin@tarides.com>
51f6364 to
8b9c066
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a fragment of #13315, separated out for easier reviewing: this is just a simple refactoring and anyone is free to have a look.
run_test_treeis the core function of ocamltest that executes a test.Justification for the change: I want to implement support for
<stmts>; !{ ... }, which should execute{ ... }if one of the statements in<stmts>skips. Determining this -- whether one of the statement skips -- is not possible/easy with the current structure ofrun_test_tree, and it is very easy with the new proposed structure.The PR is split as three small commits, it is easier to review it commit-by-commit (but maybe not too hard to review in bulk as well).