mmikeww / AHK-v2-script-converter

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

Test case 'Gui_pr_137' in failed tests ? #188

Closed andymbody closed 3 weeks ago

andymbody commented 3 weeks ago

What is this test case all about?

  1. Why is this case trying to pre-select an item in the list?
  2. What is it's criteria for choosing the item?
  3. What is the significance of items 5 and 7, 4 and 1 ?
  4. What needs to be fixed for this case, and why?

Test code:

Gui, Add, DDL,, 1|2|3|4|||||5
Gui, Add, DDL,, 6|||7|||8|||9|||
Gui, Show

Desired output:

myGui := Gui()
myGui.Add("DDL", "Choose5", ["1", "2", "3", "4", "", "", "5"])
myGui.Add("DDL", "Choose7", ["6", "", "7", "", "8", "", "9"])
myGui.Show()

Actual output:

myGui := Gui()
myGui.Add("DDL", "Choose4", ["1", "2", "3", "4", "", "", "5"])
myGui.Add("DDL", "Choose1", ["6", "", "7", "", "8", "", "9"])
myGui.Show()
andymbody commented 3 weeks ago

Never mind... I see that v1 will preselect based on the number of pipes that follow an item, and v2 requires "choose_val" instead. And the script is trying to produce same output as v1.

I have the fix ready for this case, just waiting to see if the updated fix for # 179 gets merged because my repro for this fix also has the updated changes made for # 179

mmikeww commented 3 weeks ago

I have the fix ready for this case, just waiting to see if the updated fix for #179 gets merged because my repro for this fix also has the updated changes made for #179

looks fine and i'm sure Banaanae will merge it, so you can just wait

however, a good habit to get into is, for every individual change, to start a new branch off of master, so that the new fixes wont depend on the old fixes getting merged

perhaps you did something like this:

master
      \-- edited-fix-179
                         \-- fix-188

but a better way is to checkout back to master branch first, and then branch off into a separate fix branch

      /-- fix-188
master
      \-- edited-fix-179

this way you wont have to wait for one to get merged. as long as the code doesn't overlap, there will be no conflicts. if the code does overlap, then you would need to rebase (as i mentioned in the other thread) if one gets merged in first

andymbody commented 3 weeks ago

perhaps you did something like this... but a better way is to...

Yes this is what I did. I will keep this mind going forward... thanks for your help.

Matter of fact, I could go back and redo from the current master. And submit the other PR. Need the practice anyway.

andymbody commented 3 weeks ago

Submitted PR as #189. This one is not dependent on # 179 update. Sorry for making such a mess of your github (code and comments edits, cross-links, re-openings, etc.). Still learning...

andymbody commented 3 weeks ago

a good habit to get into is, for every individual change, to start a new branch off of master, so that the new fixes wont depend on the old fixes getting merged

I think I will also make a branch that has all changes in it (merging my individual changes into a single branch) so that I can test the accumulated changes and make sure that the combination of changes does not break something (rare but could happen in some circumstances). This will also allow me to gain some experience with doing merges (within my own fork of the project). At least I think this is how it will go... I could be wrong, since I have never tried it before.

Banaanae commented 3 weeks ago

fixed in #189