Skip to content

[Console] SymfonyStyle : fix & automate block gaps. #14623

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

Merged
merged 2 commits into from
Jun 8, 2015
Merged

[Console] SymfonyStyle : fix & automate block gaps. #14623

merged 2 commits into from
Jun 8, 2015

Conversation

ogizanagi
Copy link
Contributor

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

Depends on #14741


What it does

  • autoprepend appropriate blocks (like cautions, titles, sections, ...) by the correct number of blank lines considering history.
  • handle automatically most of the SymfonyStyle guide line breaks and gaps. Fix things such as unwanted double blank lines between titles and admonitions.
  • test outputs
  • fix an issue using questions with SymfonyStyle, which should not output extra blank lines when using with a non-interactive input.

Description

SymfonyStyle is great, but there are some issues, mostly when using blocks (text blocks, titles and admonitions): some extra blank lines might be generated.

Plus, on the contrary, some line breaks or blank lines around blocks are missing, and the developer need to handle this himself by polluting his code with ugly if and newLine() statements.

Before / After :

screenshot 2015-05-13 a 00 11 59

As you can see, it's still up to the developper to end his command by a blank line (unless using a block like SymfonyStyle::success()) in order to distinct different commands outputs more efficiently.

Everything else is now handled properly, and automatically, according to the rules exposed in the symfony console style guide published some time ago by @javiereguiluz .
Questions (not exposed in the above output) are considered as blocks, and follow, for instance, the same conditions than admonitions: 1 blank line before and after, no more (although, you'll still be able to output more blank lines yourself, using newLine).

@ogizanagi
Copy link
Contributor Author

Although the implementation might not be the most elegant solution, I'd like to have some opinions about such a "feature" (in fact, I consider this an improvement just as much as a bug fix) ?
Should this behavior be configurable at SymfonyStyle instantiation ?

Fabbot issue will be fixed as soon as this PR is merged: #14620

Here is the code used in order to generate this output (+questions):

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $output = new SymfonyStyle($input, $output);

        $output->caution(array('Lorem ipsum dolor sit amet', 'consectetur adipisicing elit'));
        $output->note('Lorem ipsum dolor sit amet, ');
        $output->title('First title');
        $output->warning('Lorem ipsum dolor sit amet');

        $output->title('Second title');
        $output->section('Section One');
        $output->listing(array(
            'Lorem ipsum dolor sit amet, consectetur adipisicing elit',
            'Excepteur sint occaecat cupidatat non proident.'
        ));
        $output->write(array('<comment>Lorem ipsum dolor', 'ipsum', 'dolor</>'));
        $output->write(' amet <info>consectetur</>', true);

        $output->section('Section 2');
        $output->writeln(array('Lorem ', 'ipsum'));
        $output->write(array('Amet ', 'consectetur'));
        $output->title('Third title');

        $output->section('Section 1');
        $output->write(array('Lorem ', 'ipsum ', 'dolor '));
        $output->text(array(
            'Duis aute irure dolor in reprehenderit in voluptate velit esse',
            'cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat'
        ));

        $output->success('Success');
        $output->error('Success');

        $output->table(array('Name', 'Method', 'Scheme', 'Host', 'Path'), array(
            array('admin_post_new', 'ANY', 'ANY', 'ANY', '/admin/post/new'),
            array('admin_post_show', 'GET', 'ANY', 'ANY', '/admin/post/{id}'),
        ));

        $output->progressStart(100);
        for ($i = 0; $i < 100; $i++) {
            usleep(10000);
            $output->progressAdvance();
        }
        $output->progressFinish();

        $output->text('Duis aute irure dolor in reprehenderit in voluptate velit esse');
        $output->note($output->askHidden('Hidden question'));
        $output->note($output->choice('Choice question with default', array('choice1', 'choice2'), 'choice1'));
        $output->text('Duis aute irure dolor in reprehenderit in voluptate velit esse');;
        $output->section('Section 1');
        $output->note($output->ask('Question with default', 'default'));
        $output->write('<comment>Lorem ipsum dolor</>');
        $output->writeln(' amet <info>consectetur</>');
        $output->note($output->confirm('Confirmation with yes default', true) ? 'yes' : 'no');
        $output->note('Duis aute irure dolor in reprehenderit in voluptate velit');
    }

Tests are also provided in order to ensure the output is what we expect it to be.

Lastly, this PR also contains a bug fix about questions rendering with SymfonyStyle when using a non-interactive input. Before, extra blank lines were rendered.
As the questions themselves aren't rendered, they shouldn't output extra lines neither.
This could have been done in another PR, but as this one will cover it with tests and need it anyway, IMO it makes more sense to fix this in the very same PR.

@xabbuh xabbuh added the Console label May 14, 2015
}
//Has EOL. Split into multiples messages.
$messages = array_map(function ($message) {
return $message.PHP_EOL; //Re-add trailing EOL
Copy link
Member

Choose a reason for hiding this comment

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

This will add one more EOL than before the split, is it expected?

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if an EOL terminated $messages string can exist, which would mean an empty string here (before the concat)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will add one more EOL than before the split, is it expected?

Actually it is... But I can't remember the exact reason why. It has something to do with questions:
screenshot 2015-05-15 a 18 16 02
Before: current version, After: removing the extra EOL

However, you are right about an EOL terminated $messages string... Such code wouldn't perfectly render for instance:

 $output->write('Lorem ipsum dolor'.PHP_EOL);
 $output->note('NOTE');
 $output->write('sit amet');

The note block will not be prepended by a blank line :/

@sstok
Copy link
Contributor

sstok commented May 16, 2015

How will this affect current commands that are adding the newlines already? Just asking, as we use this for Gush at the moment :)

@ogizanagi
Copy link
Contributor Author

@sstok : If everything works as fine as I expect it to be (I need to test it on windows, soon), the only side-effect for your commands when upgrading to symfony 2.7 is that all the newlines you added manually in the aim of having proper gaps between text and block elements should be removed.

However, note that some of the newlines you might have added will have no effect if kept:

protected function execute(InputInterface $input, OutputInterface $output)
{
    $output = new SymfonyStyle($input, $output);
    $output->newLine(); //In order to have a first blank line at command start. Same effect if removed.
    $output->section('Section 1');
    $output->write('Lorem ipsum dolor');
    $output->newLine(); //Should be removed
    $output->text(array(
        'Duis aute irure dolor in reprehenderit in voluptate velit esse',
        'cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat'
    ));
    $output->newLine(); //In order to have a blank line before the success block. Same effect if removed.
    $output->success('Success');
}

Before: This piece of code without this PR.
After: This very same piece of code with this PR:
screenshot 2015-05-16 a 15 48 08

The best would be to remove any manual newlines between text and blocks elements.
Also, as mentioned above: avoid ending any string with a trailing EOL when using write (i.e, no $output->write('Lorem ipsum'.PHP_EOL)).

If your are relying on the full command output in some of your tests, there is good chances those tests fail, as some extra lines are removed (like between an admonition and a title).

I would be very pleased to receive your feedbacks if you try this code with Gush 😃

@ogizanagi
Copy link
Contributor Author

As far as I can tell, everything seems to work properly on windows concerning this PR:
screenshot 2015-05-23 a 18 00 18

I didn't investigate further, but as you can see, it seems there is a not related issue regarding strange extra lines within and after blocks.
Note that the output redirected to a file seems to be properly formatted and doesn't have any extra line.
SymfonyStyle is also mixing PHP_EOL and simple \n which leads to the use of both CRLF and simple LF under windows and will certainly make the subject of another PR, but is not sufficient in order to solve the problem.
(ANSICON isn't related either)

Edit: There is 1 extra space char per line of a block, which explains the output

@ogizanagi
Copy link
Contributor Author

This PR has been rebased onto #14741, as it indirectly improves and allows to simplify the way we keep track of an "history buffer".
Also, there is no more issue when trying to output a string with an trailing EOL using write:

$output->write('Lorem ipsum dolor'.PHP_EOL);
$output->note('NOTE');
$output->write('sit amet');

Will render as expected with this PR:

Lorem ispum dolor

! [NOTE] NOTE

sit amet

And everything works like a charm on Windows (if we forget about #14740).

ogizanagi added 2 commits May 30, 2015 12:54
Autoprepend appropriate blocks by the correct number of blank lines.
Handle most of the SymfonyStyle guide line breaks and gaps automatically and fix thing such as double blank lines between titles.
Add output tests.
Fix askQuestion method, which should not output anything when the input isn't interactive.
@ogizanagi
Copy link
Contributor Author

@fabpot : Do you think this PR is still applicable to the 2.7 branch ?
I would have liked this being part of the first release, as 2.7 introduced the SymfonyStyle class. People will now start relying on it, and this PR might obviously changes their outputs.
So, would it be considered as a BC break ? (I don't know how much people relies on the output for their tests for instance, nor if it is a good practice)

@nicolas-grekas
Copy link
Member

👍
Could go in 2.7 to me

@aitboudad
Copy link
Contributor

👍

@fabpot
Copy link
Member

fabpot commented Jun 8, 2015

Thank you @ogizanagi.

@fabpot fabpot merged commit fc598ff into symfony:2.7 Jun 8, 2015
fabpot added a commit that referenced this pull request Jun 8, 2015
…nagi)

This PR was merged into the 2.7 branch.

Discussion
----------

[Console] SymfonyStyle : fix & automate block gaps.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

---
Depends on #14741

---
## What it does
- autoprepend appropriate blocks (like cautions, titles, sections, ...) by the correct number of blank lines considering history.
- handle automatically most of the SymfonyStyle guide line breaks and gaps. Fix things such as unwanted double blank lines between titles and admonitions.
- test outputs
- fix an issue using questions with SymfonyStyle, which should not output extra blank lines when using with a non-interactive input.

## Description

`SymfonyStyle` is great, but there are some issues, mostly when using blocks (text blocks, titles and admonitions): some extra blank lines might be generated.

Plus, on the contrary, some line breaks or blank lines around blocks are missing, and the developer need to handle this himself by polluting his code with ugly `if` and `newLine()` statements.

### Before / After :

![screenshot 2015-05-13 a 00 11 59](https://cloud.githubusercontent.com/assets/2211145/7600572/ccfa8904-f90c-11e4-999f-d89612360424.PNG)

As you can see, it's still up to the developper to end his command by a blank line (unless using a block like `SymfonyStyle::success()`) in order to distinct different commands outputs more efficiently.

Everything else is now handled properly, and automatically, according to the rules exposed in the symfony console style guide published some time ago by @javiereguiluz .
Questions (not exposed in the above output) are considered as blocks, and follow, for instance, the same conditions than admonitions: 1 blank line before and after, no more (although, you'll still be able to output more blank lines yourself, using `newLine`).

Commits
-------

fc598ff [Console] SymfonyStyle : fix & automate block gaps.
260702e [Console] SymfonyStyle : Improve EOL consistency by relying on output instance
@ogizanagi ogizanagi deleted the sfstyle_block_auto_prep_blank_line branch June 8, 2015 21:02
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.

6 participants