Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve forbidIdenticalClassComparison example #104

Merged
merged 3 commits into from
Apr 21, 2023

Conversation

enumag
Copy link
Contributor

@enumag enumag commented Apr 21, 2023

Adding Brick classes here was a good inspiration, thanks for that!

However:

  • We can remove DateTimeInterface from here... the array is merged with the default configuration from rules.neon anyway so this way DateTimeInterface actually gets into ForbidIdenticalClassComparisonRule twice, I checked.
  • Brick\Money doesn't exist, it's Brick\Money\Money and I think it's better to use the interface instead anyway to also detect Brick\Money\RationalMoney.
  • BigInteger, BigDecimal and BigRational all extend BigNumber anyway so they're redundant.
  • ramsey/uuid is a very commonly used library and I almost forgot to add UuidInterface into my config.

@janedbal
Copy link
Member

It was meant as an inspiration, but working copy-pastable example is always better, thanks!

the array is merged with the default configuration from rules.neon anyway so this way DateTimeInterface actually gets into ForbidIdenticalClassComparisonRule twice, I checked

This is tricky and imo unexpected, I never realized this default Nette DI behaviour is making our "default" more a "always there" options. I think the only solution is to document that any custom setup should be done with "prevent merging" flag like blacklist!.

forbidIdenticalClassComparison:
            blacklist!: # DateTimeInterface not there anymore
                - Brick\Money\MoneyContainer
                - Brick\Math\BigNumber
                - Ramsey\Uuid\UuidInterface

@janedbal janedbal merged commit c719b30 into shipmonk-rnd:master Apr 21, 2023
9 checks passed
@janedbal
Copy link
Member

See #105

@enumag
Copy link
Contributor Author

enumag commented Apr 21, 2023

@janedbal Nice, thank you!

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.

None yet

2 participants