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

Add /silent #77

Merged
merged 25 commits into from Aug 26, 2021
Merged

Add /silent #77

merged 25 commits into from Aug 26, 2021

Conversation

SaifAqqad
Copy link
Contributor

@SaifAqqad SaifAqqad commented Jun 4, 2021

The commits do the following:

  • Add /silent which disables the MsgBoxes and instead outputs Util_Error to stderr (or stdout if stderr fails) and Util_Info to stdout.
  • Add optional verbose parameter to /silent to output Util_Status to stdout.

I have checked the "Allow edits by maintainers" box, so you can modify the changes however you see fit.

Ahk2Exe.ahk Outdated Show resolved Hide resolved
@joedf joedf requested a review from TAC109 June 4, 2021 14:51
@joedf
Copy link
Member

joedf commented Jun 4, 2021

@TAC109 @Lexikos
Thoughts? Most of the changes make sense to me, but I am hesitant with a little few of them since I have not encountered their use or case... either not yet or that I remember, such as the reload message (like #75).

@joedf joedf linked an issue Jun 5, 2021 that may be closed by this pull request
Ahk2Exe.ahk Outdated Show resolved Hide resolved
@joedf
Copy link
Member

joedf commented Jun 5, 2021

@SaifAqqad
Okay, looks pretty good to me. 👍
I'd like to wait for @TAC109 to take a quick look at it first.

@joedf joedf added the enhancement New features, improvements, etc. label Jun 5, 2021
@SaifAqqad
Copy link
Contributor Author

@joedf Now that ConsoleApp is removed I think there might be a problem with outputting errors only to stderr, as you can't see the errors with piping (| more).

@joedf
Copy link
Member

joedf commented Jun 6, 2021

@joedf Now that ConsoleApp is removed I think there might be a problem with outputting errors only to stderr, as you can't see the errors with piping (| more).

Yeah, that's the issue... If we don't use Console subsystem then the output is under a "layer" so it quite doesn't "reach" the - console output... I think there's a way around it, but it doesn't come to mind right now...

Edit: nevermind, I tried some things. didnt get it to work.. :/

@SaifAqqad
Copy link
Contributor Author

I think we could either output errors to both stdout and stderr, or we could revert it to how it was before, which is stdout only.

Ahk2Exe.ahk Outdated Show resolved Hide resolved
@Lexikos
Copy link
Contributor

Lexikos commented Jun 14, 2021

Now that ConsoleApp is removed I think there might be a problem with outputting errors only to stderr, as you can't see the errors with piping (| more).

2>&1 | more will pipe both stderr and stdout to more.

2>fileA >fileB would direct stderr to fileA and stdout to fileB.

Console apps inherit the console of the parent process if it has one, and by default have stdout and stderr set to the console. GUI apps do not have a console or any std handles by default; i.e. GetStdHandle(STD_ERROR_HANDLE) returns NULL unless the parent process specified a stderr handle when it called CreateProcess, or the process called SetStdHandle.

This will output to stderr if it exists, otherwise to stdout:

try
    FileAppend Error to stderr`n, **
catch
    FileAppend Error to stdout`n, *

@SaifAqqad
Copy link
Contributor Author

SaifAqqad commented Jun 14, 2021

This will output to stderr if it exists, otherwise to stdout:

try
    FileAppend Error to stderr`n, **
catch
    FileAppend Error to stdout`n, *

I just tested this, it works perfectly with 2>&1 | echo and with | echo

@joedf
Copy link
Member

joedf commented Jun 29, 2021

@TAC109 I'm thinking to merge this is if there are no objections. I think it's in a good state now.
I'd like your 'okay' on this, if you're available. If not, I can do further testing and do a more thorough review... to then finally approve and merge this.

@SaifAqqad
Copy link
Contributor Author

Is there any reason why this has not been reviewed by @TAC109 already?

@TAC109
Copy link
Collaborator

TAC109 commented Jul 29, 2021

@SaifAqqad Apologies for the delay, but I have been busy adding changes required for the upcoming v2 beta releases. Also attempting to squash a couple of bugs as well. I hope to review your changes soon.

Cheers

@SaifAqqad
Copy link
Contributor Author

@SaifAqqad Apologies for the delay, but I have been busy adding changes required for the upcoming v2 beta releases. Also attempting to squash a couple of bugs as well. I hope to review your changes soon.

Cheers

No worries then, take your time :D

@TAC109
Copy link
Collaborator

TAC109 commented Aug 25, 2021

@SaifAqqad
I've had a preliminary look at this PR and have noted that it includes many changes which are not directly related to the original purpose of this PR:

  • Many error messages have been changed for some reason. Ahk2Exe has it's own style of error messages which has served well in the past, and this should not be changed now.
  • For some reason this PR also includes a patch for issue Request: Add command-line option /ScriptExists=Reload | Unload | Ask #75. The author of this issue has been given several alternate solutions some of which he has found acceptable, and so there is no need for a patch to be included for this problem. I am against complicating the user interface with little-used options when there are other acceptable alternatives available.
  • There is a patch to include percent signs around the exitcode variable used in Exit and ExitApp statements. This is unnecessary as these statements accept expressions directly.

After these alterations have been made and your fork updated with the recent changes I have made here, I will review this PR in more detail.

Cheers

@SaifAqqad
Copy link
Contributor Author

@TAC109
I've applied your latest changes, restored all error messages and removed all of the /ForceReload code and other unnecessary changes (like the percent signs).

@SaifAqqad SaifAqqad changed the title Add /silent, /verbose and /ForceReload Add /silent, /verbose Aug 25, 2021
@joedf
Copy link
Member

joedf commented Aug 25, 2021

@TAC109

For some reason this PR also includes a patch for issue Request: Add command-line option /ScriptExists=Reload | Unload | Ask #75. The author of this issue has been given several alternate solutions some of which he has found acceptable, and so there is no need for a patch to be included for this problem. I am against complicating the user interface with little-used options when there are other acceptable alternatives available.

In that case I have unlinked #75 from this PR. And perhaps, we should just close #75 since suitable alternatives were presented?

@TAC109
Copy link
Collaborator

TAC109 commented Aug 26, 2021

@joedf
I agree and have closed #75

@TAC109
Copy link
Collaborator

TAC109 commented Aug 26, 2021

@SaifAqqad
Thank you for your changes. There are 2 other issues I would like you to look at:

  • Rather than having two switches /silent and /verbose I would like to see 'verbose' as an optional parameter to 'silent' such as /silent [verbose]. The SilentMode variable could then have values such as 0, 1, or 2 (with 2 being verbose), and the Verbose variable eliminated, with the code changed appropriately.
  • Also, I noted that the Ahk2Exe.ahk file on your GitHub repository has lost its BOM, causing corruption of some Unicode characters. Maybe you need to check that your editor is saving scripts as UTF-8 with BOM? Anyway, this will need to be changed.

Cheers

@SaifAqqad
Copy link
Contributor Author

Rather than having two switches /silent and /verbose I would like to see 'verbose' as an optional parameter to 'silent' such as /silent [verbose]. The SilentMode variable could then have values such as 0, 1, or 2 (with 2 being verbose), and the Verbose variable eliminated, with the code changed appropriately.

That does make more sense tbh, I'll merge your latest changes first and then do that.

@SaifAqqad
Copy link
Contributor Author

Also, I noted that the Ahk2Exe.ahk file on your GitHub repository has lost its BOM, causing corruption of some Unicode characters. Maybe you need to check that your editor is saving scripts as UTF-8 with BOM? Anyway, this will need to be changed.

You were right, vscode was saving the file with UTF-8 not UTF-8 with BOM, should be fixed now

@SaifAqqad SaifAqqad changed the title Add /silent, /verbose Add /silent Aug 26, 2021
@TAC109 TAC109 merged commit ec997fb into AutoHotkey:master Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, improvements, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CLI] Make the CLI mode completely headless
4 participants