Skip to content

Conversation

@rmand97
Copy link

@rmand97 rmand97 commented Dec 11, 2025

closes #209

Taking a stab at the issue. Hoping I understood it correctly.

@rmand97 rmand97 force-pushed the feat/expert-engine-subcommands branch from 711e699 to a7a5bdb Compare December 11, 2025 12:41
# Handle engine subcommand first (before starting the LSP server)
case argv do
["engine" | engine_args] ->
Expert.Engine.run(engine_args)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this return the exit code, and put the System.halt(exit_code) part here?

That'll make the unit tests easier to write, as well as pull the halting part up to the top level.

This way it's very clear that the program stops after this code runs.

Copy link
Author

Choose a reason for hiding this comment

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

Will do.
Just a quick question. Would you like it to always return success (0), event though it fails deleting a file?

like if its fails the middle one of 3 files in expert clean --force it will continue after failing, logging something went wrong, and delete the last one and return 0. Or would like it to stop after failing and return 1?

Copy link
Member

Choose a reason for hiding this comment

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

If it fails any, lets return a non zero exit code (1).

end

@spec print_help() :: no_return()
defp print_help do
Copy link
Member

Choose a reason for hiding this comment

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

Let's also add help text to each subcommand, and the options can be presented for the relevant subcommand.

So in the existing help text here, we won't have any options, and the --force option will show up in the help text for the clean subcommand

@rmand97 rmand97 force-pushed the feat/expert-engine-subcommands branch from c1cffc9 to 36b2874 Compare December 15, 2025 07:14
@rmand97 rmand97 force-pushed the feat/expert-engine-subcommands branch from 36b2874 to cd3a003 Compare December 16, 2025 10:57
@mhanberg
Copy link
Member

@rmand97 looks like there are some failing tests.

Comment on lines +18 to +26
case argv do
["engine" | engine_args] ->
engine_args
|> Expert.Engine.run()
|> System.halt()

_ ->
:noop
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case argv do
["engine" | engine_args] ->
engine_args
|> Expert.Engine.run()
|> System.halt()
_ ->
:noop
end
with ["engine" | engine_args] <- argv do
engine_args
|> Expert.Engine.run()
|> System.halt()
end

Comment on lines +27 to +30
OptionParser.parse(args,
strict: [force: :boolean],
aliases: [f: :force]
)
Copy link
Member

Choose a reason for hiding this comment

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

I think at this level, we would only want a help option.


case subcommand do
["ls"] -> list_engines()
["ls", options] when options in @help_options -> print_ls_help()
Copy link
Member

Choose a reason for hiding this comment

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

I believe the way to accomplish the "subcommand" cli we are wanting is to parse the options, get the subcommand, then parse the options again.

Also not we use parse_head instead of parse.

So for example

{opts, rest, _invalid} =
      OptionParser.parse_head(args,
        strict: [help: :boolean],
        aliases: [h: :help]
      )

case rest do
  ["clean" | clean_opts] -> clean_engines(clean_opts) 
  # the rest
end

defp clean_engines(argv) do
  {opts, _rest, _invalid} =
      OptionParser.parse_head(argv,
        strict: [force: :boolean, help: :boolean],
        aliases: [f: :force, h: :help]
      )

  # do the rest

end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Clean local engine build

3 participants