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

chalasr · 2016-06-22T11:38:36Z

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.

fabpot · 2016-06-22T11:40:49Z

src/Symfony/Bundle/FrameworkBundle/Command/YamlLintCommand.php


 /**
 * Validates YAML files syntax and outputs encountered errors.
 *
 * @author Grégoire Pineau <lyrixx@lyrixx.info>
 */
-class YamlLintCommand extends Command
+final class YamlLintCommand extends BaseLintCommand


final should be removed here as this is a BC break.

greg0ire · 2016-06-22T12:40:36Z

src/Symfony/Bundle/FrameworkBundle/Tests/Command/YamlLintCommandTest.php

+    {
+        $command = new YamlLintCommand();
+        $application = new Application();
+        $application->add($command);


$command becomes (hopefully) itself line 34. How about $application->add(new YamlLintCommand()) ? Wouldn't that be less confusing ?

GromNaN · 2016-06-22T13:08:10Z

src/Symfony/Component/Yaml/Command/LintCommand.php

+    protected function configure()
+    {
+        $this
+            ->setHelp($this->getFileRelatedHelp().$this->getStdinRelatedHelp())


The stdin part could be moved to the top of the help text to avoid this hackish split.

Done. BTW this should be used on the Twig LintCommand.

chalasr · 2016-06-22T13:11:14Z

Of course, the FrameworkBundle test fails because the Yaml\Command\LintCommand cannot be found in the Yaml component. Should I remove the test or is there something to do?

greg0ire · 2016-06-22T13:33:31Z

Of course, the FrameworkBundle test fails because the Yaml\Command\LintCommand cannot be found in the Yaml component. Should I remove the test or is there something to do?

What about adding it to require-dev ?

chalasr · 2016-06-22T13:35:32Z

Not sure to understand. The Yaml component is already required by the FrameworkBundle. The problem is that the command that it is looking for is provided by this PR.

(Sorry for closing, mistake)

greg0ire · 2016-06-22T13:38:28Z

Not sure to understand. The Yaml component is already required by the FrameworkBundle. The problem is that the command that it is looking for is provided by this PR.

Oh again, I read too fast, nevermind my comment.

greg0ire · 2016-06-22T13:43:02Z

YamlLintCommans => YamlLintCommand in your last commit subject (just in case the commit message somehow survives a squash)

greg0ire · 2016-06-22T13:53:55Z

Of course, the FrameworkBundle test fails because the Yaml\Command\LintCommand cannot be found in the Yaml component. Should I remove the test or is there something to do?

Link for the lazy : https://travis-ci.org/symfony/symfony/jobs/139483789#L2371

chalasr · 2016-06-22T14:20:07Z

For now I removed the FrameworkBundle YamlLintCommandTest and kept the Yaml LintCommandTest, if someone knows a trick for that, please let me know.

stof · 2016-06-22T14:31:32Z

@chalasr what is needed is to update the requirement of the component, to force FrameworkBundle to use symfony/yaml 3.2+ (older versions will not have the command, even after merging)

stof · 2016-06-22T14:34:12Z

src/Symfony/Component/Yaml/Command/LintCommand.php

+    {
+        $iterator = new \RecursiveIteratorIterator(
+            new \RecursiveDirectoryIterator($directory, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::FOLLOW_SYMLINKS),
+            \RecursiveIteratorIterator::SELF_FIRST


you should actually use LEAVES_ONLY instead of SELF_FIRST. You don't need directories in the flattened iterator, only files

stof · 2016-06-22T14:37:11Z

src/Symfony/Component/Yaml/Command/LintCommand.php

+                continue;
+            }
+
+            $files[] = $file;


rather than building an array with all files here, we could yield files to iterate over the filesystem little by little

chalasr · 2016-06-22T20:26:15Z

@stof I applied your suggestions, thank you.

Now tests pass.

…asr) This PR was merged into the 2.7 branch. Discussion ---------- [TwigBridge] Fix inconsistency in LintCommand help | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19139 (comment) | License | MIT | Doc PR | ~ Actually, the `lint:yaml` command's help looks like this: ![](http://image.prntscr.com/image/95dddf38ebcf408b8e2e23cf09c3ddf6.png) The last `Or` about STDIN is syntactically wrong, and is the only one to have a code example with indentation. This gives the same indentation for all code blocks and move the STDIN-related part as first, as proposed in #19139 (comment) . Now it looks like: ![](http://image.prntscr.com/image/8877349f6be746c981c2e9037d2d17ff.png) Commits ------- f54bb16 [TwigBridge] Fix inconsistency in LintCommand help

fabpot · 2016-06-23T05:26:10Z

src/Symfony/Component/Yaml/Command/LintCommand.php

+        }
+
+        if (!$this->isReadable($filename)) {
+            throw new \RuntimeException(sprintf('File or directory "%s" is not readable', $filename));


missing dot at the end of the message

Add tests Fix tests & YamlLintCommand help formatting fabbot fixes Use Generator to iterate over the filesystem Move STDIN related code in a method Use RecursiveIteratorIterator::LEAVES_ONLY rather than SELF_FIRST Stop using the Finder component when available (Make findFiles() private) Re-add FrameworkBundle YamlLintCommandTest Use CommandTester::getStatusCode() rather than assign execute() Re-add feature for bundle directories, Test it

chalasr · 2016-06-23T11:37:29Z

I made the changes. Re-added the @BundleName/ feature and a test for.

fabpot · 2016-06-23T11:44:47Z

Thank you @chalasr.

…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

…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

carsonbot added Status: Needs Review Feature labels Jun 22, 2016

chalasr changed the title ~~Move YamlLintCommand to the Yaml component~~ [Yaml] Move YamlLintCommand to the Yaml component Jun 22, 2016

chalasr force-pushed the standalone_yaml-lint_command branch from 4eb636b to 5317d77 Compare June 22, 2016 11:39

fabpot reviewed Jun 22, 2016
View reviewed changes

chalasr force-pushed the standalone_yaml-lint_command branch 2 times, most recently from 3e11039 to aa8f5f2 Compare June 22, 2016 12:22

chalasr mentioned this pull request Jun 22, 2016

lint:yaml command as a separated package #18987

Closed

greg0ire reviewed Jun 22, 2016
View reviewed changes

chalasr force-pushed the standalone_yaml-lint_command branch 2 times, most recently from e32eed2 to d908732 Compare June 22, 2016 12:55

GromNaN reviewed Jun 22, 2016
View reviewed changes

chalasr closed this Jun 22, 2016

chalasr reopened this Jun 22, 2016

chalasr force-pushed the standalone_yaml-lint_command branch from d908732 to 4fabb70 Compare June 22, 2016 14:06

stof reviewed Jun 22, 2016
View reviewed changes

chalasr force-pushed the standalone_yaml-lint_command branch 4 times, most recently from adeacad to f5069f3 Compare June 22, 2016 18:57

chalasr changed the title ~~[Yaml] Move YamlLintCommand to the Yaml component~~ [FramworkBundle][Yaml] Move YamlLintCommand to the Yaml component Jun 22, 2016

chalasr force-pushed the standalone_yaml-lint_command branch 2 times, most recently from e24bb86 to 5789695 Compare June 22, 2016 19:35

chalasr mentioned this pull request Jun 22, 2016

[TwigBridge] Fix inconsistency in LintCommand help #19148

Merged

fabpot reviewed Jun 23, 2016
View reviewed changes

javiereguiluz changed the title ~~[FramworkBundle][Yaml] Move YamlLintCommand to the Yaml component~~ [FrameworkBundle][Yaml] Move YamlLintCommand to the Yaml component Jun 23, 2016

javiereguiluz added the FrameworkBundle label Jun 23, 2016

chalasr force-pushed the standalone_yaml-lint_command branch 3 times, most recently from 36d7732 to 5129dbe Compare June 23, 2016 10:59

chalasr force-pushed the standalone_yaml-lint_command branch from 5129dbe to 33402ea Compare June 23, 2016 11:12

fabpot merged commit 33402ea into symfony:master Jun 23, 2016

chalasr deleted the standalone_yaml-lint_command branch June 23, 2016 12:41

This was referenced Sep 12, 2016

[Yaml] Add bin/lint for using the LintCommand #19916

Closed

[Yaml] Add lint:yaml command usage symfony/symfony-docs#6963

Closed

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chalasr commented Jun 22, 2016

fabpot Jun 22, 2016

greg0ire Jun 22, 2016

GromNaN Jun 22, 2016 •

edited

Loading

fabpot Jun 22, 2016

chalasr Jun 22, 2016

chalasr commented Jun 22, 2016

greg0ire commented Jun 22, 2016

chalasr commented Jun 22, 2016 •

edited

Loading

greg0ire commented Jun 22, 2016

greg0ire commented Jun 22, 2016

greg0ire commented Jun 22, 2016

chalasr commented Jun 22, 2016 •

edited

Loading

stof commented Jun 22, 2016

stof Jun 22, 2016

stof Jun 22, 2016

chalasr commented Jun 22, 2016

fabpot Jun 23, 2016

chalasr commented Jun 23, 2016

fabpot commented Jun 23, 2016

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

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

Conversation

chalasr commented Jun 22, 2016

fabpot Jun 22, 2016

Choose a reason for hiding this comment

greg0ire Jun 22, 2016

Choose a reason for hiding this comment

GromNaN Jun 22, 2016 • edited Loading

Choose a reason for hiding this comment

fabpot Jun 22, 2016

Choose a reason for hiding this comment

chalasr Jun 22, 2016

Choose a reason for hiding this comment

chalasr commented Jun 22, 2016

greg0ire commented Jun 22, 2016

chalasr commented Jun 22, 2016 • edited Loading

greg0ire commented Jun 22, 2016

greg0ire commented Jun 22, 2016

greg0ire commented Jun 22, 2016

chalasr commented Jun 22, 2016 • edited Loading

stof commented Jun 22, 2016

stof Jun 22, 2016

Choose a reason for hiding this comment

stof Jun 22, 2016

Choose a reason for hiding this comment

chalasr commented Jun 22, 2016

fabpot Jun 23, 2016

Choose a reason for hiding this comment

chalasr commented Jun 23, 2016

fabpot commented Jun 23, 2016

GromNaN Jun 22, 2016 •

edited

Loading

chalasr commented Jun 22, 2016 •

edited

Loading

chalasr commented Jun 22, 2016 •

edited

Loading