-
Notifications
You must be signed in to change notification settings - Fork 454
refactor: add Tests variant to Ml_sources.Origin.t #12682
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
base: main
Are you sure you want to change the base?
Conversation
1a422a3 to
a6438da
Compare
|
Where do we use the information that a module comes from a test stanza? EDIT: the change itself isn't bad, but it lacks motivation. It does not improve the code on its own, but it could be justified if it's a stepping stone for another feature or fix. I'd like to see where this is going first. |
| `Executables { Per_stanza.stanza = exes; sources; modules; obj_dir; dir } | ||
| in | ||
| let make_tests ~dir ~expander ~modules ~project tests = | ||
| let+ result = make_executables ~dir ~expander ~modules ~project tests.Tests.exes in | ||
| match result with | ||
| | `Executables group_part -> `Tests { group_part with stanza = tests } | ||
| | _ -> assert false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than reusing the make_executables function, constructing the wrong data structure, and then unwrapping it to make the updates to correct it, it seems to me like it would be cleaner to factor out the logic that produces the wrapped record Per_stanza record, then down on lines 563 and 564, have
| Executables.T exes -> `Executables (make_per_stanza ~dir ~expander ~modules ~project exes)
| Tests.T tests -> `Tests (make_per_stanza ~dir ~expander ~modules ~project tests)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be sensible if tests stanzas were a lot more different than the executables stanza, but they are really the same with one extra piece of data. The way everything is laid out means its tricky to reuse stuff due to the polymorphism. I reckon the whole thing could be ripped apart and better data structures introduced, but that's out of scope for this PR.
If you however see a way to do it, and it doesn't introduce a lot of one time used functions in the file I would be open to it, but at this point its not clear to me.
a6438da to
1a4af53
Compare
Signed-off-by: Ali Caglayan <[email protected]>
1a4af53 to
513d902
Compare
We add a way to find if an ml source comes from a
(tests)stanza. We also take the opportunity to improve the error message when there are overlapping tests / executables.