Skip to content

[Yaml] Add lint:yaml command usage #6963

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

Closed
wants to merge 3 commits into from

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Sep 12, 2016

This documents symfony/symfony#19139 (merged on 3.2) and closes symfony/symfony#19916.

~~~~~~~~~~~~~~~~~

The syntax of YAML contents can be validated through the CLI using the
:class:`Symfony\\Component\\Yaml\Command\\LintCommand` command.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should mention the console component is required (not required by the YAML one, except in dev) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it was expected.. ^^
Btw, how would you mention it? I thought about a caution

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect, applied. Thank you

@chalasr chalasr force-pushed the yaml/syntax-validation branch 2 times, most recently from 75b5639 to c4376f1 Compare September 12, 2016 21:51
@fabpot
Copy link
Member

fabpot commented Sep 12, 2016

👍

@javiereguiluz
Copy link
Member

I'm not that sure about this. Fabien is right that this is a component and you must integrate it into your setup ... but at the same time, providing a command that you cannot use unless you build a Console application looks not very friendly for the developer.

Could components provide their own commands, ready to use if you also use the Console component?

@chalasr
Copy link
Member Author

chalasr commented Sep 13, 2016

@javiereguiluz My first thought was the same as yours but indeed, it's tricky.

Having a bin/ directory in a component which doesn't contain a src/ directory feels wrong imho and, even if it's not blocking, introducing this "mistake" just for a lint command sounds slightly overkill.
On the other hand, we just introduced the same bin/ directory in the phpunit-bridge (which is not a component but has the same structure), but as it contains more logic, it seems to make more sense.

I think providing access to commands in components is already a good thing, people who want to use it just have to copy-paste this snippet. Even if not ideal, that's not a big deal.
I mean: using the components in a standalone way may require some work for reproducing features that are available in the full-stack framework (which has the console as hard dependency and provides a console application).

shall I reopen symfony/symfony/pull/19916 for discussion and deciders opinion?
cc @fabpot

@fabpot
Copy link
Member

fabpot commented Sep 13, 2016

I don't understand the last sentence of @javiereguiluz comments. We do provide the command, and it is ready to use with any console application. The snippet in this PR is just how you can create a standalone script that only validate yaml files. But most of the time, you just need to add this command to your pre-existing console app.

@chalasr chalasr force-pushed the yaml/syntax-validation branch 3 times, most recently from a636897 to 4851a2a Compare September 22, 2016 23:09
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

👍

Thanks @chalasr.


$ composer require symfony/console

Create a script containing a console application:
Copy link
Member

Choose a reason for hiding this comment

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

Create a script containing a console application: -> Create a console application with lint:yaml as its only command: ?

# or contents passed to STDIN
$ cat path/to/file.yml | php lint.php

The result will be written on STDOUT, default in TXT format.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could replace this:

The result will be written on STDOUT, default in TXT format.
For a JSON output, use the ``format`` option:

By this:

The result is written to STDOUT and uses a plain text format by default.
Add the ``--format`` option to get the output in JSON format:

@chalasr chalasr force-pushed the yaml/syntax-validation branch from 4851a2a to c4172ab Compare October 4, 2016 12:37
@chalasr
Copy link
Member Author

chalasr commented Oct 4, 2016

@javiereguiluz I applied your suggestions, thank you for them.

@xabbuh
Copy link
Member

xabbuh commented Nov 28, 2016

Thank you @chalasr.

xabbuh added a commit that referenced this pull request Nov 28, 2016
…luz)

This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes #6963).

Discussion
----------

[Yaml] Add lint:yaml command usage

This documents symfony/symfony#19139 (merged on 3.2) and closes symfony/symfony#19916.

Commits
-------

ed4cb40 Remove an unneeded code directive
44b4ad0 Use `terminal` instead of `bash` for console code
78075c1 Yaml] Add lint:yaml command usage
@chalasr
Copy link
Member Author

chalasr commented Nov 28, 2016

Looks like gh failed :). Closing as merged.
Thanks for the improvements @javiereguiluz

@chalasr chalasr closed this Nov 28, 2016
@chalasr chalasr deleted the yaml/syntax-validation branch November 28, 2016 21:33
@xabbuh
Copy link
Member

xabbuh commented Nov 28, 2016

@chalasr I didn't merge 3.2 into master yet. ;)

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