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

Deprecated not passing dash symbol (-) to STDIN commands #33496

Merged
merged 1 commit into from Sep 8, 2019

Conversation

@yceruto
Copy link
Member

commented Sep 6, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #33446 (comment)
License MIT
Doc PR -

Follow-up #33446

There's a conflict here: when no argument was provided, the command also reads from STDIN.
So now, it reads from STDIN, and if there is nothing there, reads from the default template.
This has been caught in php/php-src#4672

This creates an ambiguous situation - maybe one did pipe nothing but doesn't expect the default template dir to be linted.

I'd suggest resolving the ambiguity by reading from STDIN only when explicitly asked for. Passing - as argument could the way. And we could trigger a deprecation for now.

For consistency, the other 2 lint commands (lint:yaml and lint:xliff) have been touched as well.

The plan for 5.0 is read from the STDIN only when - is given.

/cc @nicolas-grekas

@yceruto yceruto added this to the next milestone Sep 6, 2019

@yceruto yceruto requested a review from xabbuh as a code owner Sep 6, 2019

@nicolas-grekas
Copy link
Member

left a comment

thanks :)

UPGRADE-4.4.md Outdated Show resolved Hide resolved

@yceruto yceruto force-pushed the yceruto:stdin_commands branch 2 times, most recently from 8759ffc to 00f823a Sep 7, 2019

@yceruto yceruto force-pushed the yceruto:stdin_commands branch from 00f823a to 586f299 Sep 7, 2019

@fabpot
fabpot approved these changes Sep 8, 2019
@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Thank you @yceruto.

fabpot added a commit that referenced this pull request Sep 8, 2019
feature #33496 Deprecated not passing dash symbol (-) to STDIN comman…
…ds (yceruto)

This PR was merged into the 4.4 branch.

Discussion
----------

Deprecated not passing dash symbol (-) to STDIN commands

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #33446 (comment)
| License       | MIT
| Doc PR        | -

Follow-up #33446

> There's a conflict here: when no argument was provided, the command also reads from STDIN.
So now, it reads from STDIN, and if there is nothing there, reads from the default template.
This has been caught in php/php-src#4672

> This creates an ambiguous situation - maybe one did pipe nothing but doesn't expect the default template dir to be linted.

> I'd suggest resolving the ambiguity by reading from STDIN only when explicitly asked for. Passing - as argument could the way. And we could trigger a deprecation for now.

For consistency, the other 2 lint commands (`lint:yaml` and `lint:xliff`) have been touched as well.

The plan for 5.0 is read from the STDIN only when `-` is given.

/cc @nicolas-grekas

Commits
-------

586f299 deprecated not passing dash symbol (-) to STDIN commands

@fabpot fabpot merged commit 586f299 into symfony:4.4 Sep 8, 2019

2 of 3 checks passed

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

@yceruto yceruto deleted the yceruto:stdin_commands branch Sep 8, 2019

fabpot added a commit that referenced this pull request Sep 9, 2019
minor #33509 Remove legacy code from STDIN commands (yceruto)
This PR was merged into the 5.0-dev branch.

Discussion
----------

Remove legacy code from STDIN commands

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

See #33496

Commits
-------

1994ffe remove legacy code from STDIN commands
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.