Quit after command-line parses env var

[tclose]

This PR addresses #2679 by defining an environment variable ("MRTRIX_PARSE_ONLY"), which if set to "1", will cause all commands to terminate after successful completion of the command-line parsing with a "warning" stating that the CL args have been parsed successfully.

@Lestropie @jdtournier

[tclose]

@Lestropie @jdtournier this PR is ready for review if you have a moment

[tclose]

NB: Just rebased it onto dev instead of master

[Lestropie]

Switched PR target branch to match rebasing to dev.

1. Currently early exit does not apply when running in an R environment. I don't know the extent to which that is used beyond the original implementer... nevertheless, given the definition and claimed implementation of the environment variable, it should probably be doing the same thing there also.
2. Not sure if "parse only" is adequately descriptive. In the context of CLI / workflow engine, sure, but in the context of the software as a whole, maybe something more verbose like "CLI parse only"? @jdtournier?

[tclose]

Switched PR target branch to match rebasing to dev.

1. Currently early exit does not apply when running in an R environment. I don't know the extent to which that is used beyond the original implementer... nevertheless, given the definition and claimed implementation of the environment variable, it should probably be doing the same thing there also.
2. Not sure if "parse only" is adequately descriptive. In the context of CLI / workflow engine, sure, but in the context of the software as a whole, maybe something more verbose like "CLI parse only"? @jdtournier?

Added support for Python commands that I had forgotten todo, renamed env var to MRTRIX_CLI_PARSE_ONLY. Which commands are R based?

[jdtournier]

Currently early exit does not apply when running in an R environment. I don't know the extent to which that is used beyond the original implementer...

Not at all used as far as I know... I don't think the original implementer is even still working in the field! I've been meaning to remove support for R integration, it always felt like a bit of a hack. I'll work on that when I have a minute. But in the meantime, I really wouldn't worry a great deal about this!

[Lestropie]

Which commands are R based?

All C++ commands; see parallel functions in core/command.h.

[tclose]

Which commands are R based?

All C++ commands; see parallel functions in core/command.h.

Ah, I misunderstood. You mean all mrtrix commands can be called from R, rather than R-based mrtrix commands. If it is an issue, we could just add a disclaimer that it won't work when calling from R, but it sounds like it will be dropped in any case.

Is it good to merge now? Did you request a change somewhere in the code @Lestropie? Because I can't see where that is if so

[Lestropie]

Given the absence of interest in the R interface, perhaps just clarify very briefly in the description for the new environment variable that it only applies to the shell interface and not to R?

I've left a bunch of comments in the code, and you will need to resolve conflicts with latest dev; I'll take another look after that.

[tclose]

@Lestropie Ok, I have made those changes now so I believe it should be good to re-review. Note that there is a difference in behaviour in the message printing between the c++ and python, i.e. it shows up by default in python commands but only when the -info flag is passed to c++ commands, but this seems to be the case with other "info" messages as well.

(NB: I can't see the "1 change requested", do you know what that is referring to)

[Lestropie]

it shows up by default in python commands but only when the -info flag is passed to c++ commands, but this seems to be the case with other "info" messages as well.

Uh, I suggested calling function info() here rather than warn(), thinking it was an analogue to C++ INFO() macro, but function app.info() doesn't actually exist. So I'm honestly not sure what that is even calling...

My intention was for nothing to be printed to stderr unless -info was specified. I think that if the environment variable has been set, the command is being run in a setting where they don't need to read a user-friendly message to know that parsing completed successfully and the command is exiting prematurely. There's an argument to be made that perhaps this message should be CONSOLE() / app.console()-level in case a user somehow has this envvar set unexpectedly and needs to be notified of such. And that would also make it easier to get consistent behaviour between the two languages. I'm not feeling terribly strongly either way.

I can't see the "1 change requested", do you know what that is referring to

This comment was made as a review. It'll resolve once I re-review.

[tclose]

it shows up by default in python commands but only when the -info flag is passed to c++ commands, but this seems to be the case with other "info" messages as well.

Uh, I suggested calling function info() here rather than warn(), thinking it was an analogue to C++ INFO() macro, but function app.info() doesn't actually exist. So I'm honestly not sure what that is even calling...

My intention was for nothing to be printed to stderr unless -info was specified. I think that if the environment variable has been set, the command is being run in a setting where they don't need to read a user-friendly message to know that parsing completed successfully and the command is exiting prematurely.

Maybe the code could be refactored to use Python's std logger, which would give you the behaviour you are after, but that seems like a fair bit of work for not much gain.

There's an argument to be made that perhaps this message should be CONSOLE() / app.console()-level in case a user somehow has this envvar set unexpectedly and needs to be notified of such. And that would also make it easier to get consistent behaviour between the two languages. I'm not feeling terribly strongly either way.

This was my thinking with the warning, but in either case it probably isn't worth worrying about too much

I can't see the "1 change requested", do you know what that is referring to

This comment was made as a review. It'll resolve once I re-review.

[Lestropie]

Only thing that still catches my eye is that if the contents of the envvar can't be converted to bool, the exception won't be handled neatly in C++. But realistically it's not an issue, this is a feature purely for developers anyway. So should be good to go from here.