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

PR#340 alternative ? flush -noconfirm .. exit exit_status -noconfirm #341

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dlmiles
Copy link
Contributor

@dlmiles dlmiles commented Oct 9, 2024

@wulffern please advise, test as alternative to PR#340

Documented hidden/secret exit [-noconfirm] option to disable exit prompt
Added exit [exit_status] option to send exit_status number
Added flush -noconfirm option to disable flush prompt

I have tested the 'exit' patches.

I have not yet tested the 'flush' patch. Still comment back when testing complete on my side but would appreciate feedback if it closes your concern.

Was looking to implement this feature and found it was already
implemented but is a secret feature.

Updated documentation and modified implementation so it is possible
a user can discover the feature via usage such as "quit -help -invalid"
like other commands.
Affecting process exit status.
Copy link
Owner

@RTimothyEdwards RTimothyEdwards left a comment

Choose a reason for hiding this comment

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

Using strncmp() for testing each command argument forces the arguments to be in a specific order. The preferable method is to use an option structure with the Lookup() routine, as is done for other commands with multiple options. Yes, I know I started the bad behavior. . .

@wulffern
Copy link

@dlmiles Yup, I can confirm that the flush -noprompt does what I want. Thanks.

It's a bit more finicky to use since I need to know that the -noprompt option is in the Magic version.

Magic throws an error if I use -noprompt in a Magic version that does not have it, but with a "flush_all" script like the below it works fine.

set values [ cellname list all ]
set mrev [lindex [lreverse [split [exec magic --version] "."]] 0 ]
foreach x $values {
    if {${mrev} > 495} {
        flush $x -noprompt
    } else {
        flush $x
    }
}

PS: I could not find the revision in any global tcl variable, nor was I able to redirect "version" to a variable, but I'm sure there is a more elegant way to do the above.

@RTimothyEdwards
Copy link
Owner

@wulffern : catch {flush -noprompt} should work around the backwards-compatibility failure, or maybe

if {[catch {flush -noprompt}]} {flush}

@dlmiles
Copy link
Contributor Author

dlmiles commented Oct 10, 2024

@RTimothyEdwards Did you want a revision using Lookup() ? not clear as it PR marked approved.

@wulffern maybe you can setup $ENV{'MAGIC_VERSION'} before magic is run, to TCL inherits envars, like in a startup wrapper shell script ? This way it is always available. This isn't much difference to how you are doing it now really. Maybe there should be a patch to make such a thing available from TCL an auto-populated variable.

Maybe it can be run as flush $x -noprompt and if this errors, too many args, it just retries the command without -noprompt. This may not be a possible if $x is not present, as -noprompt' maybe processed as a cellname` which will probably also error.

Difficult to provide a backward compatible mechanism that isn't as finicky, as least you can make it into myflush TCL proc to clean / hide this extra work?

Your original patch with an environment variable set from inside (or maybe inherited from outside) might be better reworked with the batch_mode feature, such that you can start up in GUI mode with batch_mode and it has the effect of making all interactive decision dialogs to cause error termination. It also make sense for $ENV{'MAGIC_BATCH_MODE'} to also be a thing so scripts a runtime. Maybe this is already possible.

Which then necessitates an option like -noprompt for every command that might present a dialog, which allows the command to succeed. This is my use case, I am looking to understand how some test cases maybe prepared that operate in GUI mode, but I need a way to manage dialog interactions and terminate with exit_status. Hence this PR.

@RTimothyEdwards
Copy link
Owner

@dlmiles : I approved it because the code is usable as-is, but if you can change it to use Lookup() I'd appreciate it.

FYI: You can use the version command in magic to get the version; it does not need to come from an external variable. Although a version --list option would be nice to get just the version numbers back as a Tcl list instead of text.

@dlmiles
Copy link
Contributor Author

dlmiles commented Oct 16, 2024

I have checked over the Lookup() methods and the usages in the codebase I can't quite see an obvious way all the requirements can be met :

  • allow -option to be any order (ok regular Lookup usage does this part)
  • continue to ensure -nodef is the same as the longer form -nodeference but also -nodefer etc.. to ensure a users script doesn't break (this is the main blocker)
  • extract the [exit_status] option, is this allowed to be in any position ? (unclear how the remaining arguments left in the list, unclaimed by any Lookup is extracted into a canonicalized form argv[]) so the arg count in this category can be seen, to allow exit -noprompt 42 but also check for invalid exit -noprompt 42 0
  • error on weird usage like exit 1 -noprompt -noprompt -nodef -nodeference maybe this is a minor one

Re current patch having fixed order, sure I added in a way that should not break any current users scripts and documentation indicates correct order to make it as easy to discover and get working as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants