NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Console] Support iterable in SymfonyStyle::write/writeln#26863

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

Jump to bottom

Merged

chalasr merged 1 commit intosymfony:masterfromogizanagi:console/sf_style_write_iterable

Apr 9, 2018

Merged

[Console] Support iterable in SymfonyStyle::write/writeln#26863

chalasr merged 1 commit intosymfony:masterfromogizanagi:console/sf_style_write_iterable

Apr 9, 2018

Conversation

Copy link

Contributor

ogizanagi commentedApr 8, 2018

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

Relates to#26847.

This would enable iterables benefits even when usingSymfonyStyle output.

[Console] Support iterable in SymfonyStyle::write/writeln

d66827e

ogizanagi added the Console label

Apr 8, 2018

ogizanagi added this to the4.1 milestone

Apr 8, 2018

carsonbot added the Status: Needs Review label

Apr 8, 2018

Copy link

Contributor

ro0NL commentedApr 8, 2018•
edited
Loading

Should/Can we updatestring|array tostring|iterable PHPdoc in StyleInterface etc?

Copy link

ContributorAuthor

ogizanagi commentedApr 8, 2018

These methods are not part ofStyleInterface, butOutputInterface which is already up to date. TheStyleInterface methods are unchanged.

Copy link

ContributorAuthor

ogizanagi commentedApr 8, 2018

(I'm not convinced enabling support for iterables inStyleInterface methods is worth it, appart maybelisting but there is already anarray typehint).

Copy link

Contributor

ro0NL commentedApr 8, 2018•
edited
Loading

StyleInterface could benefit the same upgrade (we could yield the lines fromcreateBlock()).

edit:

I'm not convinced enabling support for iterables in StyleInterface methods is worth it

Well we did it forwrite() why notsuccess() and such =/ I agree it's probably less common to use here 😅

Copy link

ContributorAuthor

ogizanagi commentedApr 8, 2018•
edited
Loading

I don't see many use cases for generators when displaying blocks, title, sections, etc... whilewrite/writeln are important to update because theSymfonyStyle can be passed along to any method/class requiring anOutputInterface and that could really benefit from generators memory saving.

chalasr approved these changes

Apr 9, 2018

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

with minor comment

...ony/Component/Console/Tests/Fixtures/Style/SymfonyStyle/command/command_4_with_iterators.php


		$output->writeln('Lorem ipsum dolor sit amet');
		$output->newLine(2);//Should append an extra blank line
		$output->title('Fifth title');

Copy link

Member

chalasrApr 9, 2018

Choose a reason for hiding this comment

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

Sixth :)

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 9, 2018

Copy link

Member

chalasr commentedApr 9, 2018

Thank you@ogizanagi.

chalasr merged commitd66827e intosymfony:master

Apr 9, 2018

chalasr added a commit that referenced this pull request

Apr 9, 2018

feature#26863[Console] Support iterable in SymfonyStyle::write/writ…

5736321

…eln (ogizanagi)This PR was merged into the 4.1-dev branch.Discussion----------[Console] Support iterable in SymfonyStyle::write/writeln| Q             | A| ------------- | ---| Branch?       | master <!-- see below -->| Bug fix?      | no| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://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   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | N/ARelates to#26847.This would enable iterables benefits even when using `SymfonyStyle` output.Commits-------d66827e [Console] Support iterable in SymfonyStyle::write/writeln

ogizanagi deleted the console/sf_style_write_iterable branch

April 9, 2018 07:51

Tobion reviewed

Apr 9, 2018

View reviewed changes

src/Symfony/Component/Console/Style/SymfonyStyle.php

		// Preserve the last 4 chars inserted (PHP_EOL on windows is two chars) in the history buffer
		returnarray_map(function ($value) {
		returnsubstr($value, -4);
		},array_merge(array($this->bufferedOutput->fetch()), (array)$messages));

Copy link

Contributor

TobionApr 9, 2018

Choose a reason for hiding this comment

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

so thefetch part was useless all along?

Copy link

ContributorAuthor

ogizanagiApr 9, 2018

Choose a reason for hiding this comment

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

It wasn't useless, but as we now only handle strings one by one and are writing to the buffer directly (instead of concatenating arrays of buffered output), it is not required anymore.