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

--dump-models-on implies --dump-models #864

Closed
wants to merge 1 commit into from

Conversation

Halbaroth
Copy link
Collaborator

If we want to dump models somewhere, we definitively want models!

I also refactor a bit the code and fix spelling.

If we want to dump models somewhere, we definitively want models!

I also refactor a bit the code and fix spelling.
@Halbaroth Halbaroth added frontend models This issue is related to model generation. labels Oct 5, 2023
Copy link
Collaborator

@bclement-ocp bclement-ocp left a comment

Choose a reason for hiding this comment

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

We may disagree on this one, but I'd prefer if we keep the option terms small, e.g. with something like

let mk_dump_models dump_models dump_models_on =
  dump_models || Option.is_some dump_models_on
in
let dump_models = Term.(const mk_dump_models $ dump_models $ dump_models_on) in

This has several advantages:

  • The huge terms such as mk_output_opt etc. where the arguments change as we add/remove options are more prone to errors, because the values we get depend on the order of the arguments in a function call. On the other hand, using small terms, calling the Options.set_XXX at the level of terms and passing () to the "final" function makes it easier to follow what an argument is doing (including using the editor's "find reference" functionality).
  • It helps keeping dependency chains narrow (e.g. here, we shouldn't have to modify the computation for interpretation at all: interpretation depends on produce_models and dump_models, so we should just be changing the way that dump_models is computed — otherwise, it is easy to forget a case or a dependency somewhere where making changes).
  • In the future, we want to be moving away from the Options module (having a single huge repository of references in a library is not good design, see [design] should remove all "sensitive" global refs in lib modules (sat solvers, ...) #377). Having small, focused terms with low responsibility makes it easier to migrate to a design where we incrementally build configuration objects rather than unit values.

@Stevendeo
Copy link
Collaborator

#864 (review)

Maybe can we merge this PR and open an issue about improving the command parsing in general which is (IMO) already quite a mess?

@bclement-ocp
Copy link
Collaborator

The PR shouldn't block additional improvements to command line parsing (which, indeed, is a mess); the behavior change itself is trivial and can easily be replicated if command line parsing is refactored; I'd rather close it (besides, there are conflicts).

@Halbaroth Halbaroth closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend models This issue is related to model generation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants