Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Contracts/Deprecation] don't use assert(), rename to trigger_deprecation()#35648

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:deprec-no-assert
Feb 8, 2020

Conversation

nicolas-grekas
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PR-

This PR is a follow up the discussion that happened in#35526.

I applied all the suggestions I received so far on this previous PR so that we can discuss them all together.

Here are the changes:

  • the most important change is tonot useassert() anymore. This fixes the objection from@ostrolucky and@derrabus that deprecations and assert() should not be bound to each other. Instead, in order to still make the function a no-op when wanted, it is suggested that apps declare an empty function. This will be more flexible in practice I believe and keeps all the benefits we envisioned by using assert();
  • as suggested by@fabpot, the function is renamedtrigger_deprecation() instead ofdeprecated(). Because the implementation is now not conditional anymore (from the package pov, since assert() is not used), my arguments to keep the previous name don't stand as strong as before to me so I'm fine renaming.
  • type hints are added back (requiring PHP 7.1 to usevoid). As mentioned in the 1st point, ppl that really want deprecations to be no-ops will redeclare the function as empty, thus not using any types, thus reclaiming all the perf diff. And for ppl that do want to catch deprecations, the overhead of types is negligible compared to any processing of the deprecations themselves.

WDYT?

All points can be discussed separately if needed of course. I'm personally fine with merging the PR as is, with all 3 changes.

ostrolucky, ro0NL, jvasseur, derrabus, and B-Galati reacted with thumbs up emoji
Copy link
Contributor

@alcaeusalcaeus left a comment

Choose a reason for hiding this comment

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

The example needs adjustment to remove the implicit cast. The rest of this is a great improvement in the implementation!

@nicolas-grekasnicolas-grekas changed the title[Contracts/Deprecation] dont use assert(), rename to trigger_deprecation()[Contracts/Deprecation] don't use assert(), rename to trigger_deprecation()Feb 8, 2020
Copy link
Contributor

@alcaeusalcaeus left a comment

Choose a reason for hiding this comment

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

Looks great!

@fabpot
Copy link
Member

Thank you for listening :)

yceruto, nicolas-grekas, OskarStark, stloyd, and derrabus reacted with heart emoji

@fabpot
Copy link
Member

Thank you@nicolas-grekas.

fabpot added a commit that referenced this pull requestFeb 8, 2020
…trigger_deprecation() (nicolas-grekas)This PR was merged into the 5.1-dev branch.Discussion----------[Contracts/Deprecation] don't use assert(), rename to trigger_deprecation()| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -This PR is a follow up the discussion that happened in#35526.I applied all the suggestions I received so far on this previous PR so that we can discuss them all together.Here are the changes:- the most important change is to *not* use `assert()` anymore. This fixes the objection from@ostrolucky and@derrabus that deprecations and assert() should not be bound to each other. Instead, in order to still make the function a no-op when wanted, it is suggested that apps declare an empty function. This will be more flexible in practice I believe and keeps all the benefits we envisioned by using assert();- as suggested by@fabpot, the function is renamed `trigger_deprecation()` instead of `deprecated()`. Because the implementation is now not conditional anymore (from the package pov, since assert() is not used), my arguments to keep the previous name don't stand as strong as before to me so I'm fine renaming.- type hints are added back (requiring PHP 7.1 to use `void`). As mentioned in the 1st point, ppl that really want deprecations to be no-ops will redeclare the function as empty, thus not using any types, thus reclaiming all the perf diff. And for ppl that do want to catch deprecations, the overhead of types is negligible compared to any processing of the deprecations themselves.WDYT?All points can be discussed separately if needed of course. I'm personally fine with merging the PR as is, with all 3 changes.Commits-------0032b2a [Contracts/Deprecation] don't use assert(), rename to trigger_deprecation()
@fabpotfabpot merged commit0032b2a intosymfony:masterFeb 8, 2020
@nicolas-grekasnicolas-grekas deleted the deprec-no-assert branchFebruary 8, 2020 13:46
@derrabus
Copy link
Member

Thank you very much! 😃

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jdreesenjdreesenjdreesen requested changes

@fabpotfabpotfabpot approved these changes

@alcaeusalcaeusalcaeus approved these changes

@ycerutoycerutoyceruto approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

7 participants
@nicolas-grekas@fabpot@derrabus@alcaeus@jdreesen@yceruto@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp