Skip to content

[master][console] Allow multiple options to be set. #19495

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 1 commit into from
Sep 14, 2016

Conversation

SpacePossum
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
License MIT

This PR add the possibility to set multiple options on a formatted string output.
Example:

$output->writeln('<fg=green;options=bold,underscore>Test</>');

Secondly it makes the behavior on invalid values consistent.

// current
$output->writeln('<fg=lime;>Test</>'); // throws exception
$output->writeln('<options=italic;>Test</>'); // silent ignore

// new
$output->writeln('<fg=lime;>Test</>'); // throws exception
$output->writeln('<options=italic;>Test</>'); // throws exception

All other changes are about making the code more strict or other SCA/CS fixes.

@fabpot
Copy link
Member

fabpot commented Jul 31, 2016

Anything not related to this new feature should be removed as it would make merging old branches more difficult (and they don't bring any value anyway).


/**
* @group legacy
* @dataProvider provideInlineStyleTagsWithUnknownOptions
Copy link
Member

Choose a reason for hiding this comment

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

missing @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered annotation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done,
just for my understanding, why is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@SpacePossum when tests run in max depth (latest versions of the components supported for an older package) this function might not be available from the bridge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense, thanks for the info @HeahDude !

@SpacePossum SpacePossum force-pushed the master_console_output_options branch from 11649ef to 243d6fc Compare August 29, 2016 11:05
@SpacePossum SpacePossum force-pushed the master_console_output_options branch from 4cee435 to 1430138 Compare August 29, 2016 11:13
@SpacePossum
Copy link
Contributor Author

rebased and all comments addressed

@fabpot
Copy link
Member

fabpot commented Sep 14, 2016

Thank you @SpacePossum.

@fabpot fabpot merged commit 1430138 into symfony:master Sep 14, 2016
fabpot added a commit that referenced this pull request Sep 14, 2016
…pacePossum)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[master][console] Allow multiple options to be set.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| License       | MIT

This PR add the possibility to set multiple options on a formatted string output.
Example:
```php
$output->writeln('<fg=green;options=bold,underscore>Test</>');
```

Secondly it makes the behavior on invalid values consistent.
```php
// current
$output->writeln('<fg=lime;>Test</>'); // throws exception
$output->writeln('<options=italic;>Test</>'); // silent ignore

// new
$output->writeln('<fg=lime;>Test</>'); // throws exception
$output->writeln('<options=italic;>Test</>'); // throws exception
```

All other changes are about making the code more strict or other SCA/CS fixes.

Commits
-------

1430138 Allow multiple options to be set.
xabbuh added a commit to symfony/symfony-docs that referenced this pull request Sep 21, 2016
…SpacePossum)

This PR was merged into the master branch.

Discussion
----------

[Console] Add multiple options for the output example

With symfony/symfony#19495 merged to master a way to set multiple options has been added.

Commits
-------

c6b4738 Update coloring.rst
@fabpot fabpot mentioned this pull request Oct 27, 2016
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.

5 participants