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

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

Merged
merged 1 commit into from Apr 9, 2018

Conversation

Projects
None yet
5 participants
@ogizanagi
Member

ogizanagi commented Apr 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 using SymfonyStyle output.

@ogizanagi ogizanagi added the Console label Apr 8, 2018

@ogizanagi ogizanagi added this to the 4.1 milestone Apr 8, 2018

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 8, 2018

Should/Can we update string|array to string|iterable PHPdoc in StyleInterface etc?

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 8, 2018

These methods are not part of StyleInterface, but OutputInterface which is already up to date. The StyleInterface methods are unchanged.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 8, 2018

(I'm not convinced enabling support for iterables in StyleInterface methods is worth it, appart maybe listing but there is already an array typehint).

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 8, 2018

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

edit:

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

Well we did it for write() why not success() and such =/ I agree it's probably less common to use here 😅

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 8, 2018

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

@chalasr

chalasr approved these changes Apr 9, 2018

with minor comment

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

This comment has been minimized.

@chalasr

chalasr Apr 9, 2018

Member

Sixth :)

@chalasr

This comment has been minimized.

Member

chalasr commented Apr 9, 2018

Thank you @ogizanagi.

@chalasr chalasr merged commit d66827e into symfony:master Apr 9, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

chalasr added a commit that referenced this pull request Apr 9, 2018

feature #26863 [Console] Support iterable in SymfonyStyle::write/writ…
…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     <!-- 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   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Relates to #26847.

This would enable iterables benefits even when using `SymfonyStyle` output.

Commits
-------

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

@ogizanagi ogizanagi deleted the ogizanagi:console/sf_style_write_iterable branch Apr 9, 2018

{
// We need to know if the two last chars are PHP_EOL
// Preserve the last 4 chars inserted (PHP_EOL on windows is two chars) in the history buffer
return array_map(function ($value) {
return substr($value, -4);
}, array_merge(array($this->bufferedOutput->fetch()), (array) $messages));

This comment has been minimized.

@Tobion

Tobion Apr 9, 2018

Member

so the fetch part was useless all along?

This comment has been minimized.

@ogizanagi

ogizanagi Apr 9, 2018

Member

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.

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 9, 2018

Nice change @ogizanagi . Thanks

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