mmikeww / AHK-v2-script-converter

AHK v1 -> v2 script converter
https://autohotkey.com/boards/viewtopic.php?f=6&t=25100
The Unlicense
538 stars 42 forks source link

MaxIndex() and MinIndex() method not converted properly. #178

Open srourbrendan opened 4 months ago

srourbrendan commented 4 months ago

In v1, MaxIndex() is an object method, therefore supported by arrays. In v2, the Array class does not have a MaxIndex() or MinIndex() method, causing converted code to give errors.

v1 code:

F1::
x := [1, , 3]
MsgBox % x.MaxIndex()
Return

Generated v2 code:

F1::
{
x := [1, , 3]
MsgBox(x.MaxIndex())
Return
} 

Running the generated code as is will give the following error:

Error: This value of type "Array" has no method named "MaxIndex".

Note: MaxIndex() does exist in the v2 docs, as a method for the ComObjArray class.

In my case, I manually converted x.MaxIndex() to x.Length. While they are technically different conceptually, functionally they yield the same value. I'm not sure if that works as a full solution though.

Banaanae commented 4 months ago

Maybe something like this?

x := [1, , 3]
MsgBox x.Length != 0 ? x.Length : ""

x := []
MsgBox x.Length != 0 ? x.Length : ""

and for MinIndex

x := [1, , 3]
MsgBox x.Length != 0 ? 1 : ""

x := []
MsgBox x.Length != 0 ? 1 : ""

Afaik Max returns length and and Min return 1 unless array is blank? If so then this should work

Banaanae commented 4 months ago

^ MaxIndex() is fine But MinIndex fails this example

MsgBox % {-10: 0, -1: 0}.MinIndex()  ;  -10
mmikeww commented 4 months ago

^what does MaxIndex return in that last example?

Banaanae commented 4 months ago
MsgBox % {-10: 0, -1: 0}.MaxIndex()  ;  -1
MsgBox % {-10: 0, -1: 0}.Length()  ;  0

These are more complicated than I originally thought, and may not have a perfect match in v2

mmikeww commented 4 months ago

yeah for now best to prob do nothing

Banaanae commented 4 months ago

Actually since map properties and array properties are converted separately, it's possible to use this solution just for arrays, then leave maps how they are for now

andymbody commented 4 months ago

Suggestion...

How about if the converter adds these custom properties (once) to the script when it encounters references to MinIndex() or MaxIndex()... And a note that communicates that Min/MaxIndex() should not be used for future scripts because it is not directly supported by v2. This will ensure that it is supported by the converted script, but also informs the user that this will not always be the case for new scripts.

Array.Prototype.DefineProp('MinIndex', {Call:(this)=>(i:='',e:=this.__Enum(),[((*)=>e(&k,&v)&&(i:=(i=''||k<i)?k:i))*],i)})
Array.Prototype.DefineProp('MaxIndex', {Call:(this)=>(i:='',e:=this.__Enum(),[((*)=>e(&k,&v)&&(i:=(i=''||i<k)?k:i))*],i)})

; Array example
a := []
MsgBox '[' a.minindex() ']' ; (empty string)
MsgBox '[' a.maxindex() ']' ; (empty string)
a := [100,200,300]
MsgBox '[' a.minindex() ']' ; [1]
MsgBox '[' a.maxindex() ']' ; [3]

Should work for maps as well, with an extra check

Map.Prototype.DefineProp('MinIndex',{Call:(this)=>(i:='',e:=this.__Enum(),[((*)=>e(&k,&v)&&(i:=(isNumber(k)&&(i=''||k<i))?k:i))*],i)})
Map.Prototype.DefineProp('MaxIndex',{Call:(this)=>(i:='',e:=this.__Enum(),[((*)=>e(&k,&v)&&(i:=(isNumber(k)&&(i=''||i<k))?k:i))*],i)})

; Map example
m := Map()
MsgBox '[' m.minindex() ']' ; (empty string)
MsgBox '[' m.maxindex() ']' ; (empty string)
m := Map(-10,0, -1,0, -5,0, -11,0, "key5","val5")
MsgBox '[' m.minindex() ']' ; [-11]
MsgBox '[' m.maxindex() ']' ; [-1]

Although, AFAIK, this object literal is not valid in v2 obj := {-10: 0, -1: 0}

Banaanae commented 4 months ago

This would work, but it feels like it goes against the purpose of this script. Like in #100 the main purpose of the script is to ease the transition to into V2.

However we could link this issue, so if people do need MinIndex and MaxIndex they could add it themselves?

@mmikeww do you have any ideas?

andymbody commented 4 months ago

This would work, but it feels like it goes against the purpose of this script. Like in #100 the main purpose of the script is to ease the transition to into V2.

However we could link this issue, so if people do need MinIndex and MaxIndex they could add it themselves?

Yeah... it is stepping slightly out of bounds, but there is no direct conversion for Min/MaxIndex() between v1 and v2. This would allow the converted script to support these unsupported methods. Which helps ease the burden of manual edit for the user.

I did add this solution to the script section of the forum in case anyone wants to add it manually.

mmikeww commented 4 months ago

my initial reaction is that that is kinda out of scope for the converter. but i dont really know how it should handle it

andymbody commented 4 months ago

I read the comments for #100. I see that once an exception is made, it may lead to an expectation for other situations to be supported as well. I wonder if a secondary tool might be better to address some of these "exceptions" that can be run after the conversion with this tool? Just thinking out loud...

Banaanae commented 3 months ago

I read the comments for #100. I see that once an exception is made, it may lead to an expectation for other situations to be supported as well. I wonder if a secondary tool might be better to address some of these "exceptions" that can be run after the conversion with this tool? Just thinking out loud...

It could be possible to add these inside the Quick convertor menu, we can make a PR to add these changes, as there's atleast two other issues that could benefit from this

Banaanae commented 3 months ago

image

Made a quick script for this and found MinIndex fails with this example MsgBox % [ , , 1, 2, 3].MinIndex() ; 3 but above fix returns 1

andymbody commented 3 months ago

Made a quick script for this and found MinIndex fails with this example MsgBox % [ , , 1, 2, 3].MinIndex() ; 3 but above fix returns 1

I misunderstood 'Returns the lowest or highest integer key, if present.'

See below for the corrected versions...

andymbody commented 3 months ago

ok... here is the corrected custom properties that should return the same value as v1

Array.Prototype.DefineProp('MinIndex', {Call:(a)=>(i:='',e:=a.__Enum(),[((*)=>e(&k,&v)&&(i:=k,!IsSet(v)))*],i)})
Array.Prototype.DefineProp('MaxIndex', {Call:(a)=>(i:='',e:=a.__Enum(),[((*)=>e(&k,&v)&&(i:=IsSet(k)?k:i))*],i)})
; examples
MsgBox [,"two","three",,"five",].MinIndex() ;2
MsgBox [,"two","three",,"five",].MaxIndex() ;5
MsgBox [ , , 1, 2, 3].MinIndex()        ;3
MsgBox [ , , 1, 2, 3].MaxIndex()        ;5
MsgBox [].MinIndex()                ; (empty string)
MsgBox [].MaxIndex()                ; (empty string)

I think a map is stored with keys in alphabetical order, so not sure how helpful Min/MaxIndex will be for maps. But it does work, if you decide to include Min/MaxIndex for maps.

Banaanae commented 2 months ago

Update on this issue: Object methods and array methods can be converted separately We can use https://github.com/mmikeww/AHK-v2-script-converter/issues/178#issuecomment-2141335817 for arrays (MinIndex will require a warning as it isn't a 100% fix) Objects are unlikely to receive a fix (just too different from v1) The fix for arrays is challenging as with the current setup we can only edit MaxIndex() in MyArray.MaxIndex() and can't access the required MyArray bit (as my solution requires it twice), still possible to fix but we need to do it post convert