Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Document the weak_vendors value#7453
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
greg0ire commentedFeb 7, 2017
Code PR:symfony/symfony#21539 |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be more descriptive imho
components/phpunit_bridge.rst Outdated
| also set the value ``"weak"`` which will make the bridge ignore any deprecation | ||
| notices. This is useful to projects that must use deprecated interfaces for | ||
| backward compatibility reasons. | ||
| backward compatibility reasons, or for libraries with lots of vendors that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Lots of is undefined to the reader, it doesn't help one understand if that applies to its use cases or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess one dependency with many deprecations is enough indeed.
components/phpunit_bridge.rst Outdated
| notices. This is useful to projects that must use deprecated interfaces for | ||
| backward compatibility reasons. | ||
| backward compatibility reasons, or for libraries with lots of vendors that | ||
| can create a sudden backlog of deprecations to fix that would otherwise prevent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same for sudden imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I want to convey the fact that this can happen without any code change in the lib itself, because dependencies are not locked.
components/phpunit_bridge.rst Outdated
| backward compatibility reasons. | ||
| backward compatibility reasons, or for libraries with lots of vendors that | ||
| can create a sudden backlog of deprecations to fix that would otherwise prevent | ||
| unrelated pull requests from being merged… which leads us to ``"weak_vendors"``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ellipses suggest the reader understood, which might be not true yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Replaced the ellipsis withdescribed below so that the reader doesn't feel like they missed something.
nicolas-grekas commentedFeb 7, 2017
(but not longer ;) ) |
greg0ire commentedFeb 7, 2017
I reworked it a bit. Reviewers, feel free to point me to parts that you feel are too verbose / superfluous. |
| the Composer lock file, which would create another class of issues. Libraries | ||
| will often use ``SYMFONY_DEPRECATIONS_HELPER=weak`` because of this. This has | ||
| the drawback of allowing contributions that introduce deprecations but: | ||
| * forget to fix the deprecated calls if there are any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does not render as a bullet list because of missing new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch! Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The list still doesn't render properly, seehttps://github.com/greg0ire/symfony-docs/blob/weak_vendors/components/phpunit_bridge.rst#making-tests-fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Woops, I think I amended a very old commit and force pushed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed again
This PR was merged into the 3.3-dev branch.Discussion----------Introduce weak vendors modeDeprecations coming from the vendors are segregated from otherdeprecations. A new mode is introduced, in which deprecations comingfrom the vendors are not taken into account when deciding to exit withan error code.| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MIT| Doc PR |symfony/symfony-docs#7453<!--- Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too).- Features and deprecations must be submitted against the master branch.- Please fill in this template according to the PR you're about to submit.- Replace this comment by a description of what your PR is solving.-->Sample output :# Weak vendor mode## With both vendors and non-vendor errors```WARNINGS!Tests: 1068, Assertions: 2714, Warnings: 6, Skipped: 15.Remaining deprecation notices (1)some error: 1x 1x in SonataAdminBundleTest::testBuild from Sonata\AdminBundle\TestsRemaining vendor deprecation notices (4)Legacy deprecation notices (48)```Exit code is 1## After fixing non-vendor errors```WARNINGS!Tests: 1068, Assertions: 2714, Warnings: 6, Skipped: 15.Remaining vendor deprecation notices (4)Legacy deprecation notices (48)```Exit code is 0# TODO- [x] fix colorization issues (vendor deprecation notices are always in red)- [x] make the vendor detection more robust- [x] make the vendor dir configurable- [x] ~figure out how to run tests and~ add more of them - [x] test on non-vendor file - [x] test on vendor file- [x] do not change the output of other modesCommits-------61fd043 Introduce weak vendors mode
greg0ire commentedFeb 28, 2017
Please remove the [WCM] tag and review this |
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
Really well explained! Thanks@greg0ire
greg0ire commentedFeb 28, 2017
Glad you like it@javiereguiluz ! |
components/phpunit_bridge.rst Outdated
| backward compatibility reasons. | ||
| backward compatibility reasons, or for libraries that do not want deprecations | ||
| triggered by their dependencies to prevent unrelated pull requests from being | ||
| merged, which leads us to the ``"weak_vendors"`` value, described below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would revert the changes to this paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why? What's wrong with trying to make a smooth transition like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After more thinking, maybe you're right, I'm kind of explaining the same thing twice here.
xabbuh commentedMar 13, 2017
Thank you@greg0ire. |
This PR was merged into the master branch.Discussion----------Document the weak_vendors valueSeesymfony/symfony#21539Commits-------abe88c6 Document the weak_vendors value
Uh oh!
There was an error while loading.Please reload this page.
Seesymfony/symfony#21539