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

POC Tokens::insertSlices #4250

Merged
merged 1 commit into from Jan 25, 2021
Merged

Conversation

keradus
Copy link
Member

@keradus keradus commented Jan 9, 2019

ref #4221
cc @kubawerlos , this is what we discussed in priv

@keradus keradus changed the title Tokens::insertSlices POC Tokens::insertSlices Jan 9, 2019
@keradus keradus changed the base branch from 2.14 to 2.12 January 9, 2019 01:07
\strlen($foo);
\strlen($foo);
',
'<?php
Copy link
Member Author

@keradus keradus Jan 9, 2019

Choose a reason for hiding this comment

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

This, unchanged Tokens collection is size of 20.
The fixer has to add 3 new tokens to this collection.

original code has to run offsetSet 45 times, as it has to move all tokens from strlen, then middle strlen, then from json_encode - which means that last strlen was moved 3 times!
my proposal has to run offsetSet only 20 times, as it has to move one token no more than once

Basically, this gives the possibility to reduce O(n) of calling offsetSet
from O(x*y)
to O(x+y)
where x is number of existing tokens and y is number of new tokens to insert

@keradus
Copy link
Member Author

keradus commented Jan 18, 2019

why I believe this is a good idea?
I pushed a fix for #3996 to this PR
see the time difference without and with this PR:
#3996 (comment)

Copy link
Contributor

@kubawerlos kubawerlos left a comment

Choose a reason for hiding this comment

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

Is the name insertSlices fitting situation? Quite often (if not most times) we will use this to insert array of tokens. Wouldn't insertMultiple (or insertMany) better describe what the function is doing?

src/Tokenizer/Tokens.php Outdated Show resolved Hide resolved
src/Tokenizer/Tokens.php Outdated Show resolved Hide resolved
src/Tokenizer/Tokens.php Outdated Show resolved Hide resolved
Copy link
Contributor

@dmvdbrugge dmvdbrugge left a comment

Choose a reason for hiding this comment

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

Functionality seems good, just some minor points, up to you if you want to fix them.

Copy link
Contributor

@mvorisek mvorisek left a comment

Choose a reason for hiding this comment

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

Any progress on this?

Before merging check if the tests files can be reduced, I think the issue is not related to the overall length, so all keys/values can be probably changed to single char like 0 (zero) or '' (empty string).

@keradus
Copy link
Member Author

keradus commented Apr 16, 2020

@julienfalque , @SpacePossum , what do you think about the proposal in this PR? especially do we want to promote new method as new "best practice" or not?
https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/4250/files#diff-0ae0077906c99aaf35085e05c46cefd3R284

I think the issue is not related to the overall length, so all keys/values can be probably changed to single char like 0 (zero) or '' (empty string).

it can, i was just too lazy to modify example provided by original reporter.

@keradus keradus changed the base branch from 2.15 to 2.16 November 20, 2020 15:22
@keradus keradus changed the base branch from 2.16 to 2.17 December 17, 2020 13:40
@keradus keradus changed the base branch from 2.17 to 2.18 January 24, 2021 21:36
@keradus
Copy link
Member Author

keradus commented Jan 24, 2021

Is the name insertSlices fitting situation? Quite often (if not most times) we will use this to insert array of tokens. Wouldn't insertMultiple (or insertMany) better describe what the function is doing?

for me, insertMultiple/insertMany is more fitting actual behaviour of insertAt, where we are pointing a single index, and adding multiple tokens to it:

$tokens->insertAt(15, $tokensCollection)
$tokens->insertAt(10, [$tokenA, $tokenB]);
$tokens->insertAt(5, $tokenC);

I'm trying to express that we are inserting different slices to different indexes at single call:

$tokens->insertSlices([
    5 => $tokenC,
    10 => [$tokenA, $tokenB],
    15 => $tokensCollection,
]);

we can always change the name later, as it's marked as internal

@coveralls
Copy link

coveralls commented Jan 24, 2021

Coverage Status

Coverage increased (+0.006%) to 91.929% when pulling 07cf8b0 on keradus:2.12_insert_multi into be542fb on FriendsOfPHP:2.18.

@keradus keradus merged commit 4a142ed into PHP-CS-Fixer:2.18 Jan 25, 2021
@keradus keradus added this to the 2.18.2 milestone Jan 25, 2021
This was referenced Mar 11, 2021
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.

None yet

5 participants