Skip to content

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

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 8, 2019

Conversation

yceruto
Copy link
Member

@yceruto yceruto 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

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

thanks :)

@yceruto yceruto force-pushed the stdin_commands branch 2 times, most recently from 8759ffc to 00f823a Compare September 7, 2019 12:10
@fabpot
Copy link
Member

fabpot commented Sep 8, 2019

Thank you @yceruto.

fabpot added a commit that referenced this pull request Sep 8, 2019
…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
@yceruto yceruto deleted the stdin_commands branch September 8, 2019 11:36
fabpot added a commit that referenced this pull request Sep 9, 2019
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
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
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.

4 participants