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

make doSaveDependingOnFiles nullable again #25

Merged
merged 2 commits into from
Dec 9, 2019
Merged

Conversation

dfx413
Copy link
Contributor

@dfx413 dfx413 commented Dec 3, 2019

addresses #23

@dfx413 dfx413 force-pushed the master branch 4 times, most recently from 255ea03 to 28360cd Compare December 3, 2019 09:36
Copy link
Member

@Spamercz Spamercz left a comment

Choose a reason for hiding this comment

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

Nullable fix is OK. But class aliases should not be in this. Please remove that so I can merge this.

@@ -3,6 +3,6 @@
declare(strict_types = 1);

return [
\Kdyby\DoctrineCache\NotImplementedException::class => \Kdyby\DoctrineCache\Exception\NotImplementedException::class,
\Kdyby\DoctrineCache\Exception::class => \Kdyby\DoctrineCache\Exception\Exception::class,
\Kdyby\DoctrineCache\Exception\NotImplementedException::class => \Kdyby\DoctrineCache\Exception\NotImplementedException::class,
Copy link
Member

Choose a reason for hiding this comment

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

This should not be here. You are aliasing same class name. This is here just to provide backwards compatibility for renamed namespaced class \Kdyby\DoctrineCache\NotImplementedException

\Kdyby\DoctrineCache\NotImplementedException::class => \Kdyby\DoctrineCache\Exception\NotImplementedException::class,
\Kdyby\DoctrineCache\Exception::class => \Kdyby\DoctrineCache\Exception\Exception::class,
\Kdyby\DoctrineCache\Exception\NotImplementedException::class => \Kdyby\DoctrineCache\Exception\NotImplementedException::class,
\Throwable::class => \Throwable::class,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@dfx413 dfx413 requested a review from Spamercz December 8, 2019 17:21
@dfx413
Copy link
Contributor Author

dfx413 commented Dec 8, 2019

Hello, sorry for inconvenience. Should be OK now.

@Spamercz
Copy link
Member

Spamercz commented Dec 9, 2019

No worries :). Merging

@Spamercz Spamercz merged commit 5888873 into Kdyby:master Dec 9, 2019
@greeny
Copy link

greeny commented Aug 13, 2020

Can this be tagged, please?

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.

3 participants