Implement `__debugInfo` magic method

[francislavoie]

If I use Symfony's VarDumper to write out a BigDecimal (e.g. with psysh or whatever when debugging), all we see is something like this:

> $transaction->getAmountDecimal()
= Brick\Math\BigDecimal {#7066}

If BigDecimal implemented the __debugInfo() magic method, it would be visible.

For example, if we add this:

    public function __debugInfo(): ?array
    {
        return [
            'value' => $this->value,
            'scale' => $this->scale,
        ];
    }

We instead see in the output:

> $transaction->getAmountDecimal()
= Brick\Math\BigDecimal {#7064
    value: "10000",
    scale: 2,
  }

Would there be any negative side effects to this?

[BenMorel]

Hi @francislavoie, I can't see any negative side effects to adding __debugInfo, apart from the fact that it may only be useful for Symfony, as I think that PHP itself would always output private properties in var_dump().

Also, if I run it in isolation, I can see that Symfony's dump() also dumps the private properties by default:

Do you know the rules used by VarDumper to decide whether or not to display properties? Are we sure that adding __debugInfo() will always force it to output them?

[francislavoie]

Ah, right. It's cause psysh turns on some flags to hide protected & private properties. They use Caster::filter($a, Caster::EXCLUDE_PROTECTED | Caster::EXCLUDE_PRIVATE). They appear if they're in debugInfo though because I think that effectively promotes it to public, I think?

I have another workaround for now though, VarDumper has configurable "Casters" which are callbacks which return a map of properties. I decided to do this for our own use for now:

    /**
     * @param BigDecimal $decimal
     * @return array
     */
    public static function castBigDecimal($decimal)
    {
        return [
            Caster::PREFIX_VIRTUAL.'string' => (string) $decimal,
        ];
    }

Ultimately the value/scale are not that relevant for our own use so I decided to use the string cast which has less mental overhead to read at a glance.

I did notice that pretty much the only way to get the value&scale is to call ->serialize() which is marked @internal so it felt dirty. Maybe there should be a ->getValue() to match ->getScale()?

(Totally off-topic, I was instinctually looking for ->times() and ->add() but instead there's ->multipliedBy() and ->plus(), feels asymmetrical to me :man_shrugging: would be cool if those aliases existed I think)

[BenMorel]

Ultimately the value/scale are not that relevant for our own use so I decided to use the string cast which has less mental overhead to read at a glance.

For VarDumping, I agree that a string representation is a better fit as it's meant to be human readable.

I have another workaround for now though, VarDumper has configurable "Casters" which are callbacks which return a map of properties

I think this is the way to go, this is problably something that you want to configure in your application, outside of brick/math!

Maybe there should be a ->getValue() to match ->getScale()?

There's getUnscaledValue() which probably fits your needs (this returns a BigInteger though, so you would need to cast it to string).

(Totally off-topic, I was instinctually looking for ->times() and ->add() but instead there's ->multipliedBy() and ->plus(), feels asymmetrical to me :man_shrugging: would be cool if those aliases existed I think)

I'm a bit against having multiple methods that do the same thing, I like that 2 codebases (or worse, 2 parts of the same codebase) can't use different method names for the same thing, for the sake of consistency.

Especially, I'm against add() as, IMO, it might imply mutability, whereas plus() sounds more logical to me for something that returns a new value and leaves the original value unchanged.

feels asymmetrical to me

Can you please expand on this?

[francislavoie]

There's getUnscaledValue() which probably fits your needs (this returns a BigInteger though, so you would need to cast it to string).

Ah yeah that works 👍 I'm sure I saw it but it didn't immediately return a string so I just read past it I think.

feels asymmetrical to me

Can you please expand on this?

What I mean is multipliedBy is past tense & has the By which doesn't match with plus which is present tense (I guess?) and has no By.

I think times() would align better. Or going the other way, incrementedBy (or something like that) instead of plus... but that sounds crazy 😅

I dunno, it just feels kinda long having a 12 character method name when an equivalent 5 character one could exist. Both are a lot more verbose than * (if we had operator overloading... we can still dream).

Anyway I think we can close this because I think we agree __debugInfo isn't needed. But 🤷‍♂️ I think there's still some room for UX improvements for the method names probably, not that important though.