search

brick / money

A money and currency library for PHP
MIT License
1.61k stars 96 forks source link

Allow using Money, Currency and Context in Psalm's immutable contexts #75

Open someniatko opened 1 year ago

someniatko commented 1 year ago

The motivation of this PR is the inability to use the value objects this lib provides in projects which use Psalm as a static analyzer, in its "immutable" contexts.

Added @psalm-immutable and @psalm-pure annotations to appropriate classes and static methods.

I took some shortcuts: for example, for the places where \NumberFormatter is used, psalm-suppress annotations was required.

Also, unfortunately, Currency::of() is using ISOCurrencyProvider which is heavily impure internally, therefore all static methods using Currency::of() were not marked as pure. Even more unfortunate is that one of the Money's non-static method is using Currency::of() and this place had to be marked with a @psalm-suppress. I am not sure what to do about this.

someniatko commented 1 year ago

The ISOCurrencyProvider is of course just providing the information hardcoded into the lib, but uses optimization trickery which is "impure". Maybe marking the two usages of this provider in the Currency::of() and Currency::ofCountry() as @psalm-suppress ImpureMethodCall and marking the methods themselves as @psalm-pure is an okay-ish tradeoff?

From the Psalm's point of view it would be ideal to force the client of the library to call the provider explicitly (basically, treating the provider as a sort of a repository), and/or the other classes should not accept currency codes and automatically transform them into Currency instances from the provider, instead only take the Currency instance, but this arguably is a worse developer experience.

someniatko commented 1 year ago

Before you merge it, what do you think about this problem:

The ISOCurrencyProvider is of course just providing the information hardcoded into the lib, but uses optimization trickery which is "impure". Maybe marking the two usages of this provider in the Currency::of() and Currency::ofCountry() as @psalm-suppress ImpureMethodCall and marking the methods themselves as @psalm-pure is an okay-ish tradeoff?

BenMorel commented 9 months ago

The ISOCurrencyProvider is of course just providing the information hardcoded into the lib, but uses optimization trickery which is "impure". Maybe marking the two usages of this provider in the Currency::of() and Currency::ofCountry() as @psalm-suppress ImpureMethodCall and marking the methods themselves as @psalm-pure is an okay-ish tradeoff?

I guess marking Currency::of() as pure and muting Psalm here is OK. As I understand it, even if its implementation is not pure, this method still exhibits a pure behaviour?

someniatko commented 9 months ago

I guess marking Currency::of() as pure and muting Psalm here is OK. As I understand it, even if its implementation is not pure, this method still exhibits a pure behaviour?

I think yes, since the source data used by the ISOCurrencyProvider is used as immutable, but just lazy-loaded.

I will add @psalm-pure annotation then.