Skip to content

Comments

BUGFIX: Ternary with nested variable access in condition#10

Merged
grebaldi merged 4 commits intomainfrom
bugfix/ternary-with-nested-variable-access-in-condition
Jun 5, 2023
Merged

BUGFIX: Ternary with nested variable access in condition#10
grebaldi merged 4 commits intomainfrom
bugfix/ternary-with-nested-variable-access-in-condition

Conversation

@mhsdesign
Copy link
Contributor

@mhsdesign mhsdesign commented Apr 8, 2023 •
edited
Loading

previously:

true === someStruct.foo ? "a" : "foo" is not correctly transpiled. (Its valid ecmascript see the ast that should be build: https://astexplorer.net)

Failed asserting that two strings are equal.
    Expected :'((true === $this->someStruct->foo) ? 'a' : 'foo')'
    Actual   :'(true === ($this->someStruct->foo ? 'a' : 'foo'))'

i looked now a long time at this code, and still dont know if this is the right solution or if we need to fix the precedence of something ...

@mhsdesign
Copy link
Contributor Author

mhsdesign commented Apr 8, 2023 •
edited
Loading

Needs to be probably adjusted for optchain too (once #11 is merged)

Nope should work out of the box ^^

@mhsdesign mhsdesign requested a review from grebaldi April 23, 2023 13:41
@mhsdesign
BUGFIX: check $precedence in expression parsing loop
a7b9ece 
I have the feeling it's the right way. It just fell like this into place. Cant 100% explain why but after debugging it i think it is right.
@mhsdesign mhsdesign changed the title WIP: BUGFIX: Ternary with nested variable access in condition BUGFIX: Ternary with nested variable access in condition Apr 24, 2023
@mhsdesign mhsdesign marked this pull request as ready for review April 24, 2023 08:30
grebaldi
grebaldi approved these changes Jun 5, 2023
View reviewed changes
Copy link
Member

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

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

Hi @mhsdesign,

thanks for fixing this!

I saw that php-stan was complaining about the missing return type of ternaryOperationWithVariablesInConditionExamples. So I took the liberty of adding it. Along the way I also added two more examples to the data provider to stress-test the solution. Works superbly! 👍

test/Unit/Target/Php/Transpiler/TernaryOperation/TernaryOperationTranspilerTest.php

/**
* @dataProvider ternaryOperationExamples
* @dataProvider ternaryOperationWithVariablesInConditionExamples
Copy link
Member

@grebaldi grebaldi Jun 5, 2023

Choose a reason for hiding this comment

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

TIL :) I did not know this was possible :D

@grebaldi grebaldi merged commit 2c99694 into main Jun 5, 2023
@grebaldi grebaldi deleted the bugfix/ternary-with-nested-variable-access-in-condition branch June 5, 2023 12:13
@grebaldi
Copy link
Member

grebaldi commented Jun 5, 2023

Yeah... I ran the wrong tests... I'm an idiot 😅

Will fix in main soon

@grebaldi grebaldi mentioned this pull request Jun 5, 2023
Merged
mhsdesign added a commit that referenced this pull request Aug 6, 2023
@mhsdesign
TASK: Cleanup ExpressionNode.php
62d7da3 
The additions that here from #18 are not necessary. The condition is also handled by the "while" loop and later comes the return. 

I think i messed stuff up because the partial fix belongs to #10

either way, it works as it is, just not super clean, thats why i removed the duplicate logic.
@mhsdesign mhsdesign mentioned this pull request Aug 6, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grebaldi grebaldi grebaldi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mhsdesign @grebaldi