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

CS fixes #28814

Merged
merged 1 commit into from Oct 11, 2018
Merged

CS fixes #28814

merged 1 commit into from Oct 11, 2018

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Oct 11, 2018

Q A
Branch? 2.8
Bug fix? no
New feature? o
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

@@ -41,8 +41,8 @@ public function __construct(ContainerInterface $container)
*
* @param string $eventName The name of the event to dispatch. The name of the event is
Copy link
Member

Choose a reason for hiding this comment

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

Don't we then need to change the description here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but out of the scope of this PR. I just want the code to have 0 CS errors. If someone wants to improve the current description, they are more than welcome, but I would not see that as a priority.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I was just wondering why the fixer complained about one case while leaving the other untouched.

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, I don't understand why this is not fixed. /cc @SpacePossum @keradus

@fabpot fabpot merged commit d48a377 into symfony:2.8 Oct 11, 2018
fabpot added a commit that referenced this pull request Oct 11, 2018
This PR was merged into the 2.8 branch.

Discussion
----------

CS fixes

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | no
| New feature?  | o <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - 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.
-->

Commits
-------

d48a377 fixed CS
@keradus
Copy link
Member

keradus commented Oct 11, 2018

Hi @fabpot , thanks for note.
This is bug that we had sadly introduced in last release of 2.12.

If description is a multiple sentences, rule shall not change it.


This is good change:

-@param string $foo Description of param.
+@param string $foo description of param

This is not a good change:

-@param string $foo Description of param. Multiple sentences.
+@param string $foo description of param. Multiple sentences

This bug will be fixed in next 2.12 release, see PR to see state of work on that:
PHP-CS-Fixer/PHP-CS-Fixer#4027

@keradus
Copy link
Member

keradus commented Oct 11, 2018

This PR follows after tool, but it introduces bad changes, that won't happen anymore after fix on PHP CS Fixer side.
I would suggest to revert it.

SpacePossum added a commit to PHP-CS-Fixer/PHP-CS-Fixer that referenced this pull request Oct 18, 2018
This PR was squashed before being merged into the 2.12 branch (closes #4027).

Discussion
----------

PhpdocAnnotationWithoutDotFixer - add failing cases

introduced in #3882

ref symfony/symfony#28814
ref symfony/symfony#28817

Commits
-------

330fe39 PhpdocAnnotationWithoutDotFixer - add failing cases
@keradus keradus mentioned this pull request Oct 20, 2018
nicolas-grekas added a commit that referenced this pull request Oct 30, 2018
This PR was merged into the 2.8 branch.

Discussion
----------

Revert "fixed CS"

This reverts commit d48a377.

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | n/a
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

This PR reverts #28814 , that was caused as a bug of PHP CS Fixer fixed in PHP-CS-Fixer/PHP-CS-Fixer#4027

After fix on PHP CS Fixer side, the rule is passing now at Symfony's codebase.

This PR only reverts wrong chances done by PHP CS Fixer,
it does not apply new rule requested in #28817 ( PHP-CS-Fixer/PHP-CS-Fixer#4045 )

Commits
-------

6f83d9f Revert "fixed CS"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants