Skip to content

Conversation

@Orest-Divintari
Copy link

There is a use case where via rector we convert an integer from this format 1050 to 10_50 which represents for example $10.50

This was supported by rector but after this pr that is not supported anymore.

If you create a new Int_(10_50) the printer will print it as 1050

So i created this pr to add it here if that makes sense
If not, could you please let me know if there is an alternative solution to separate with _ integer values ?

@theofidry
Copy link
Contributor

There is a version of the printer for which the node formatting is preserved (within reason). Maybe that could be done there automatically rather than hiding it behind an undocumented flag.

@Orest-Divintari
Copy link
Author

There is a version of the printer for which the node formatting is preserved (within reason). Maybe that could be done there automatically rather than hiding it behind an undocumented flag.

Thanks for replying!
I want to clarify the use case: I'm not trying to preserve existing formatting, but rather transform the formatting of parsed integers.

For example, in a Cash context where amounts are in cents:

  • Original: new Cash(1050)
  • Desired output: new Cash(10_50) (for readability of $10.50)

@ruudk
Copy link
Contributor

ruudk commented Oct 31, 2025

There is a version of the printer for which the node formatting is preserved (within reason). Maybe that could be done there automatically rather than hiding it behind an undocumented flag.

It's true, when using the preserving formatter, this is kept. But that's the point. We want to change existing integers and format them using underscores. That means that we don't want to preserve, want a complete new representation. Therefore, the preserving formatter does not have a role.

$node = Int_::fromString('12_34');

// object(PhpParser\Node\Scalar\Int_)#2 (2) {
//  ["attributes":protected]=>
//  array(2) {
//    ["rawValue"]=>
//    string(5) "12_34"
//    ["kind"]=>
//    int(10)
//  }
//  ["value"]=>
//  int(1234)
// }
var_dump($node);

echo new Standard()->prettyPrintExpr($node); // returns 1234 while we expect 12_34

So there is no way with the Standard printer, and plain AST, to write it to format 12_34.

@ruudk
Copy link
Contributor

ruudk commented Oct 31, 2025

@samsonasik What do you think of this? I think it makes a lot of sense. Would be great if you could do a review as well.

Copy link
Contributor

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

This should apply on float values as well?

$kind = $node->getAttribute('kind', Scalar\Int_::KIND_DEC);
if (Scalar\Int_::KIND_DEC === $kind) {
return (string) $node->value;
return $node->getAttribute('shouldPrintRawValue')
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be interesting to see how much slower this additional calls will be when pretty-printing a reasonable big code file (especially for those not opt-in into the feature)

Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea how to prevent the slowdown?

Copy link
Contributor

@samsonasik samsonasik Oct 31, 2025

Choose a reason for hiding this comment

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

how about add something like Kind::RAW_VALUE ?

Copy link
Author

Choose a reason for hiding this comment

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

i used the raw value approach

Copy link
Contributor

@ruudk ruudk Nov 7, 2025

Choose a reason for hiding this comment

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

@staabm Thanks for the review, would you mind having another look at the updated PR? Would be great to have your approval too. Thanks 🙏

@Orest-Divintari
Copy link
Author

This should apply on float values as well?

the float method does not reach to the kind attribute
If the concern for performance regarding the int method was to reach to another attribute
Does that apply here as well ?

Meaning,
Will it slow down if we check for the kind attribute in the float method ?

Copy link
Contributor

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

I think this can be starter, another scalar values can follow this if this merged 👍

*/
public static function fromString(string $str, array $attributes = [], bool $allowInvalidOctal = false): Int_ {
if(isset($attributes['kind']) && $attributes['kind'] === self::KIND_RAW_VALUE){
return new Int_((int) $str, $attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think rawValue attribute need to be added as well here, ensure rawValue is added as well before returned.

Suggested change
return new Int_((int) $str, $attributes);
$attributes['rawValue'] = $str;
return new Int_((int) $str, $attributes);

so the value is always there to avoid changed on reprinting later.

if needed, move $attributes['rawValue'] = $str; to early before the if.

Copy link
Author

Choose a reason for hiding this comment

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

$str is not the raw value
if you override the raw value with $str then this new change will do nothing
the goal is to be able to set the raw value yourself and print that instead of $str
$attributes['rawValue'] = $str;

Copy link
Contributor

Choose a reason for hiding this comment

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

okay 👍 let's see @nikic opinion about this or if there is better way for this :)

@ruudk
Copy link
Contributor

ruudk commented Nov 13, 2025

@nikic I know you are probably very busy, but if you have time, would you mind giving this a review? This will help us a lot in Rector. Thanks 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants