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

Fix deprecation ignore by link #31

Closed
wants to merge 2 commits into from
Closed

Conversation

janlanger
Copy link

Initial value in ignoredLinks must be set to 1, same as https://github.com/doctrine/deprecations/blob/master/lib/Doctrine/Deprecations/Deprecation.php#L143 Otherwise the error is not ignored.

1234
);

$this->assertEquals(1, Deprecation::getUniqueTriggeredDeprecationsCount());
Copy link
Member

Choose a reason for hiding this comment

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

🤔 if the deprecation is ignored, why is it taken into account in the count?

Copy link
Author

Choose a reason for hiding this comment

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

It's the same test as the ane above for ignore by package. I don't know the reason behind this.

Copy link
Member

Choose a reason for hiding this comment

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

This test does not make sense to me, and in fact, the existing code makes no sense to me either.

/** @var array<string,bool> */
private static $ignoredPackages = [];
/** @var array<string,int> */
private static $ignoredLinks = [];

Why is ignoredLinks a hash of counters instead of a hash of booleans?

@derrabus
Copy link
Member

derrabus commented Aug 3, 2022

Closing because the PR is stalling. Feel free to open a new one if you want to resume the work.

@derrabus derrabus closed this Aug 3, 2022
@janlanger
Copy link
Author

@derrabus There is nothing to work on, the PR is ready to be reviewed and merged

@derrabus derrabus reopened this Aug 5, 2022
@derrabus
Copy link
Member

derrabus commented Aug 5, 2022

All right, reopening then. It's just that you had not answered @greg0ire's question until a couple of minutes ago.

But I must admit, I don't understand your change and the test confuses me even more.

@greg0ire
Copy link
Member

greg0ire commented Aug 5, 2022

But I must admit, I don't understand your change and the test confuses me even more.

For me it's not really the test that I don't understand, it's the pre-existing code 😅

It feels like ignoreLinks should be a hash of booleans, and that there should be another property perhaps named triggeredDeprecations which would be a hash of counters. @beberlei , maybe you can help with this?

@janlanger
Copy link
Author

It's just that you had not answered @greg0ire's question until a couple of minutes ago.

But I must admit, I don't understand your change and the test confuses me even more.

Well, yes, because the question is about pre-existing code and not the change :-) I've just fixed obvious bug, based on the $ignoredLinks usage elsewhere, and adapted test from existing one. I cannot answer previous architecture choices...

@derrabus
Copy link
Member

All right. Can you produce a green test suite?

@ruudk
Copy link
Contributor

ruudk commented Jun 2, 2023

@derrabus @greg0ire I applied the above feedback in #43

@stof
Copy link
Member

stof commented Jun 2, 2023

Closing in favor of #43

@stof stof closed this Jun 2, 2023
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