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

lint:yaml command as a separated package #18987

Closed
soullivaneuh opened this issue Jun 7, 2016 · 28 comments · Fixed by #34387
Closed

lint:yaml command as a separated package #18987

soullivaneuh opened this issue Jun 7, 2016 · 28 comments · Fixed by #34387
Labels
Feature Good first issue Ideal for your first contribution! (some Symfony experience may be required) Yaml

Comments

@soullivaneuh
Copy link
Contributor

soullivaneuh commented Jun 7, 2016

Currently, if we want to use the lint:yaml command, we can only do this on a Symfony Full Stack installation, or require FrameworkBundle and add the command under a self made application file.

It would be great if we can have this one as a binary, like php-cs-fixer for example.

This would be useful to integrate it under Travis for example, for our bundles.

An equivalent already exists for Ruby: https://github.com/Pryz/yaml-lint

Or maybe move it under the yaml component?

What do you think?

@greg0ire
Copy link
Contributor

greg0ire commented Jun 7, 2016

Is lint:yaml about warnings or only errors ? Because if it is only errors, I am not sure we will catch anything with such a check…

@greg0ire
Copy link
Contributor

greg0ire commented Jun 7, 2016

If it show deprecations it could be worth it though… for instance, not quoting service ids has been deprecated lately.

@GromNaN
Copy link
Member

GromNaN commented Jun 12, 2016

The command twig:lint was decoupled from the framework in #9855. It must be as easy for yaml:lint.
I see only 1 line that depends on the FrameworkBundle.

@stof
Copy link
Member

stof commented Jun 15, 2016

yeah, and this is only about supporting things like @FooBundle/...

@GromNaN
Copy link
Member

GromNaN commented Jun 20, 2016

The last question is : in which component this generic command should be placed ? Currently, it depends on Console, Yaml and Finder.

IMO, the Yaml component is the best candidate to host this command because that's a feature of Yaml.

@soullivaneuh
Copy link
Contributor Author

IMO, the Yaml component is the best candidate to host this command because that's a feature of Yaml.

Agree. And what about having a binary?

@fabpot
Copy link
Member

fabpot commented Jun 21, 2016

I would vote for the Yaml component as well.

@chalasr
Copy link
Member

chalasr commented Jun 21, 2016

Is it convenient to introduce a class in the Yaml component that depends on the Console+Finder ones?
It wasn't a problem for the TwigLintCommand as twig has it's own bridge suggesting a large set of components.

I personally made a private port of the YamlLintcommand as a single-command app (yaml-lint binary) based on the 3 required components so I'm 👍 for having this, but maybe outside of the symfony components. Maybe open a small binary based library like the php-cs-fixer, as suggested by @soullivaneuh.

@stof
Copy link
Member

stof commented Jun 21, 2016

the finder dependency could be avoided actually, by using SPL iterators directly without the nice API of the Finder (should be easy as the command just does a recursive iteration and then filters files by extension. It does not rely on fancy features of the Finder).

@chalasr
Copy link
Member

chalasr commented Jun 22, 2016

In fact a standalone library providing only a linter sounds overkill.
See #19139 for moving it in the Yaml component.

fabpot added a commit that referenced this issue Jun 23, 2016
…ml component (chalasr)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[FrameworkBundle][Yaml] Move YamlLintCommand to the Yaml component

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

See #18987 for the use case.
Also I see several things that can be simplified/optimized in the original command, but I think it would be better to propose the changes in a next PR and just propose the exact changes needed to move it outside of the framework. If all should be done here, let me know before reviewing it.

Commits
-------

33402ea Add Yaml LintCommand
@fabpot fabpot closed this as completed Jun 23, 2016
@soullivaneuh
Copy link
Contributor Author

@chalasr It's a good start, but in this case we still have to create the console file calling this command.

I still think having a ready-to-use package is a good idea. WDYT it's overkill?

@chalasr
Copy link
Member

chalasr commented Feb 24, 2017

@soullivaneuh I quickly changed my mind on that.
I proposed to add it as executable to the yaml component in #19916.
Unfortunately it has been rejected because components aren't meant to provide ready-to-use executables.
Finally, we added the content of the executable in the docs so that anyone can copy-paste it.

But I still think you're right about that having it as a package makes sense. It looks like there is a similar intent for twig btw: twigphp/twig-lint

@javiereguiluz
Copy link
Member

I agree with @soullivaneuh's proposal. I'd love something simple that I can use in Travis CI and deployment scripts. Thanks!

@xabbuh
Copy link
Member

xabbuh commented Feb 26, 2017

I was looking into creating a small package for this. But in fact there already is one: https://github.com/certificationy/yaml-linter

@javiereguiluz
Copy link
Member

@xabbuh I wasn't asking for a separate package/component/bundle/etc. I just need that the component itself provides a ready-to-use linter binary. Something like this:

$ php .../vendor/symfony/.../translation/Resources/bin/lint.php

The "binary" would be a minimal Symfony console app with just 1 command.

@soullivaneuh
Copy link
Contributor Author

@javiereguiluz I prefer the .phar solution, but this could be enough too. 😉

@chalasr
Copy link
Member

chalasr commented Jun 21, 2017

👍 for putting such binaries into the Resources directory of each component

@chalasr
Copy link
Member

chalasr commented Mar 27, 2018

We now provide binaries in components (e.g. #26654), reopening.

@chalasr chalasr reopened this Mar 27, 2018
@fabpot fabpot added the Good first issue Ideal for your first contribution! (some Symfony experience may be required) label Aug 6, 2019
@fabpot fabpot added this to Fabien's in Help Wanted Aug 6, 2019
@mickaelandrieu
Copy link
Contributor

Hello @chalasr,

is something like that useful?

https://github.com/certificationy/yaml-linter

How can I help with that?

Mickaël

@fabpot
Copy link
Member

fabpot commented Aug 6, 2019

Another one: https://github.com/asm89/twig-lint

@derrabus
Copy link
Member

derrabus commented Aug 6, 2019

What would be the goal of a separate package? When I recently made an attempt to improve the linting capabilities of the YAML component (#30137), it was rejected because there are already better linters out there.

@chalasr
Copy link
Member

chalasr commented Aug 6, 2019

To me, we only need to provide an executable in the Yaml component.
That executable would create a console application, wire the LintYamlCommand and run it.
The docs shows how to write such binary on your own, so closing here would mean removing that example from the docs and replace it by "just use the lint binary".

Syntax validation is part of the Yaml component features, there is no point in making it a dedicated package as soon as the component provides the code needed to use it.

@igor-susic
Copy link

If this issue is available I would gladly implement this one :) @fabpot @chalasr

@mickaelandrieu
Copy link
Contributor

The docs shows how to write such binary on your own, so closing here would mean removing that example from the docs and replace it by "just use the lint binary".

Contributing the lint.php file directly into the Yaml component is enough to fix this issue or do you need more?

@chalasr
Copy link
Member

chalasr commented Aug 19, 2019

Yes, nothing more to me. PR welcomed from anyone, could be nice to let @igor-susic doing it for its first contribution.

@igor-susic
Copy link

@chalasr If others agree, I would like to be assigned and deal with this one 😄

@jschaedl
Copy link
Contributor

@igor-susic Are you still working on this one? I would be happy to provide a PR in case you don't have time.

@flobb
Copy link

flobb commented Dec 13, 2019

Another one https://github.com/HeahDude/yaml-linter

@fabpot fabpot closed this as completed Jan 11, 2020
fabpot added a commit that referenced this issue Jan 11, 2020
This PR was merged into the 5.1-dev branch.

Discussion
----------

[Yaml] Added yaml-lint binary

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #18987  <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | tbd.
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

Commits
-------

2640dfe [Yaml] Introduce yaml-lint binary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Good first issue Ideal for your first contribution! (some Symfony experience may be required) Yaml
Projects
None yet
Development

Successfully merging a pull request may close this issue.