Simplify conversions to remove errorlevel checks where possible

[slycordinator]

Currently, if a script uses ErrorLevel to check for the success/failure of the prior command, the converter changes it to store in an ErrorLevel variable then use that in the ensuing commands. It works, but the variable is unneccesary as in V2, the command success/failure can be checked directly in an if statement. Granted, this is refactoring rather than strictly conversion, but it would be a nice addition, imo.

V1 code:

Process, Exist, notepad.exe
if !ErrorLevel {
    Run notepad.exe
}

V2 from conversion:

ErrorLevel := ProcessExist("notepad.exe")
if !ErrorLevel {
    Run("notepad.exe")
}

V2 simplified:

if not ProcessExist("notepad.exe") {
    Run("notepad.exe")
}

V1 code:

Process, Exist, other.exe
if (ErrorLevel == 0) {
    MsgBox Process other.exe already exists!
}

V2 from conversion:

ErrorLevel := ProcessExist("other.exe")
if (ErrorLevel == 0) {
    MsgBox("Process other.exe already exists!")
}

V2 simplified:

if ProcessExist("other.exe") {
    MsgBox("Process other.exe already exists!")
}

[mmikeww]

The problem is that its not a simple conversion like that. The user could use the ErrorLevel variable in many different ways. Sure, if the v1 code looks exactly like your examples and only checks ErrorLevel in an if-statement, and never checks the variable ever again, then your conversion might be possible. But what if the v1 code uses ErrorLevel in multiple places? Or in a ternary?

Possible v1 codes:

Process, Exist, other.exe
(ErrorLevel == 0) ?? myMsg("other.exe already exists") : myMsg("other.exe doesn't exist")

myMsg(text) {
   MsgBox, %text%
}

  Hotkey, %temp%,, UseErrorLevel
  if ErrorLevel in 5,6

Or what if a user creates and sets ErrorLevel themselves? Like in [https://www.autohotkey.com/board/topic/116561-lib-ini-v10-basic-ini-string-functions/](this ini library)

    if (!ini_insertKey(ini, SectionName, name . "=" . value))
    {
      MsgBox, 16, DtS, Setting Save Failed `ninsertKey %ErrorLevel%
      return
    }

;...


ini_insertKey(ByRef _Content, _Section, _Key)
{
    StringLeft, K, _Key, % InStr(_Key, "=") - 1
    sectionCopy := ini_getSection(_Content, _Section)
    keyList := ini_getAllKeyNames(sectionCopy)
    isInserted = 0
    If K Not In %keyList%
    {
        sectionCopy .= "`n" . _Key
        isInserted = 1
    }
    If isInserted
    {
        ini_replaceSection(_Content, _Section, sectionCopy)
        ErrorLevel = 0 
    }
    Else
    {
        ErrorLevel = 1
    } 
    Return isInserted
}

Overall this suggestion seems very difficult to do and probably very difficult to do correctly. If you feel like trying to implement it in a bug-free manner, of course your Pull Request would be accepted.

[slycordinator]

I can understand why that could fit better in a linter than a converter script. I'll look into how this functionality could be added.

Though, it's possibly of note that in the above V1 code, including the line with (ErrorLevel == 0) ?? myMsg("other.exe already exists") : myMsg("other.exe doesn't exist") results in the converter returning nothing (blank output).

[mmikeww]

Though, it's possibly of note that in the above V1 code, including the line with (ErrorLevel == 0) ?? myMsg("other.exe already exists") : myMsg("other.exe doesn't exist") results in the converter returning nothing (blank output).

i made an error, the ternary should only have one question mark, not two

[user1823]

I am not a dev and don't understand the above conversation well. But, here's my observation:

v1 code:

#g::
GetText(Temp)
If NOT ERRORLEVEL
{
    Run, msedge.exe "https://www.google.com/search?q=%Temp%"
}
Return

GetText is a function that copies selected text.

converted code:

#g::
{ ; V1toV2: Added bracket
GetText(Temp)
If NOT ERRORLEVEL
{
    Run("msedge.exe `"https://www.google.com/search?q=" Temp "`"")
}
Return
} ; V1toV2: Added bracket in the end

While trying to run this, I get two errors:

Warning: This variable appears to never be assigned a value.
Specifically: local Temp

Warning: This variable appears to never be assigned a value.
Specifically: local ERRORLEVEL

I managed to solve this by converting the code to:

#g::
{ ; V1toV2: Added bracket
Temp := GetText()
{
    Run("msedge.exe `"https://www.google.com/search?q=" Temp "`"")
}
Return
} ; V1toV2: Added bracket in the end

I know the Temp / GetText issue is a separate one, but I don't know how to describe it. So, any of the readers, please create a new issue for that.

[mmikeww]

Warning: This variable appears to never be assigned a value.
Specifically: local Temp

Warning: This variable appears to never be assigned a value.
Specifically: local ERRORLEVEL

I know the Temp / GetText issue is a separate one

its actually the same issue, and it would also occur in AHK v1 if you used #Warn. but your code is a different concept than this github issue

[user1823]

but your code is a different concept than this github issue

I am unable to understand it conceptually. You can make a new issue for this with proper description.

it would also occur in AHK v1 if you used #Warn.

After reading the following, I don't think that v1 will give a warning about ERRORLEVEL.

v1 had a global value called ErrorLevel, which roughly mimics return codes from shell programs. If you tried calling SoundPlay but it couldn’t play the sound, AHK would set ErrorLevel to 1.

Now there’s an obvious problem with this, which is that it was global state changed by everything. You could never be 100% sure that the ErrorLevel you read came from the command you expected it.

Source: https://www.hillelwayne.com/post/ahk-v2/

[mmikeww]

correct, v1 should only give the warning for Temp not ErrorLevel