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

Change transformers order, fixing untransformed T_USE #4104

Open
wants to merge 4 commits into
base: 2.12
from

Conversation

Projects
None yet
3 participants
@dmvdbrugge
Contributor

dmvdbrugge commented Nov 13, 2018

Fixes #4086

@dmvdbrugge dmvdbrugge force-pushed the dmvdbrugge:4086-untransformed-T_USE branch from efbf69f to dd80318 Nov 13, 2018

@dmvdbrugge dmvdbrugge force-pushed the dmvdbrugge:4086-untransformed-T_USE branch 3 times, most recently from ae239d0 to 106677d Nov 13, 2018

@keradus

This comment has been minimized.

Member

keradus commented Nov 21, 2018

please, add code sample provided in #4086 to NoUnusedImportsFixerTest.php as well, that way:

  • we will have a proof that fix to Transformer fixes the Fixer
  • when one would change Transformer logic, we will be sure that Fixer still works properly

@keradus keradus added this to the 2.12.5 milestone Nov 21, 2018

foreach ($tokens as $index => $token) {
foreach ($this->items as $transformer) {
foreach ($this->items as $transformer) {
foreach ($tokens as $index => $token) {

This comment has been minimized.

@keradus

keradus Nov 21, 2018

Member

I checked and I have no better idea for this fix.
Yet, it's huge risk to change that order. We can bring with this change similar issues to one you are just fixing - if in other place one Transformer relies on other Transformer to not yet transform sth...

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Nov 21, 2018

Contributor

You are partially correct, however the priority already suggests one transformer should run before another, which is now even better enforced.

@dmvdbrugge dmvdbrugge force-pushed the dmvdbrugge:4086-untransformed-T_USE branch from 106677d to 01c441e Nov 21, 2018

@dmvdbrugge

This comment has been minimized.

Contributor

dmvdbrugge commented Nov 21, 2018

please, add code sample (...) to NoUnusedImportsFixerTest.php as well

Done, and I also added it to OrderedClassElementsFixerTest as that suffers from the same issue (which explains "Weird thing 2").

@keradus

This comment has been minimized.

Member

keradus commented Nov 21, 2018

for next time - please don't squash commits of yours,
now, one needs to re-review everything, instead of just few fixes you made according to comments :(

@dmvdbrugge

This comment has been minimized.

Contributor

dmvdbrugge commented Nov 22, 2018

for next time - please don't squash commits of yours,
now, one needs to re-review everything, instead of just few fixes you made according to comments :(

My apologies, I had misinterpreted some earlier comments in this repo. I usually don't even do it this way. Won't happen again.

@julienfalque julienfalque added the bug label Nov 24, 2018

@keradus

This comment has been minimized.

Member

keradus commented Nov 25, 2018

don't worry, nothing big (squashing or not). sometimes, when we can't split ourselves, we ask original creator to squash, but it is very rarely happening

Show resolved Hide resolved src/Tokenizer/Transformers.php Outdated

@dmvdbrugge dmvdbrugge force-pushed the dmvdbrugge:4086-untransformed-T_USE branch from ea66e98 to 3c8a6d4 Dec 6, 2018

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