refactor: add Tests variant to Ml_sources.Origin.t#12682
refactor: add Tests variant to Ml_sources.Origin.t#12682Alizter wants to merge 1 commit intoocaml:mainfrom
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.
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.
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 <alizter@gmail.com>
1a4af53 to
513d902
Compare
|
Closing in favour of #12785 |
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.