Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle][Yaml] Move YamlLintCommand to the Yaml component#19139
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4eb636b to5317d77Compare| * @author Grégoire Pineau <lyrixx@lyrixx.info> | ||
| */ | ||
| class YamlLintCommandextendsCommand | ||
| finalclass YamlLintCommandextendsBaseLintCommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
final should be removed here as this is a BC break.
3e11039 toaa8f5f2Compare| { | ||
| $command = new YamlLintCommand(); | ||
| $application = new Application(); | ||
| $application->add($command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$command becomes (hopefully) itself line 34. How about$application->add(new YamlLintCommand()) ? Wouldn't that be less confusing ?
e32eed2 tod908732Compare| protected function configure() | ||
| { | ||
| $this | ||
| ->setHelp($this->getFileRelatedHelp().$this->getStdinRelatedHelp()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The stdin part could be moved to the top of the help text to avoid this hackish split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done. BTW this should be used on the Twig LintCommand.
chalasr commentedJun 22, 2016
Of course, the FrameworkBundle test fails because the |
greg0ire commentedJun 22, 2016
What about adding it to |
chalasr commentedJun 22, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedJun 22, 2016
Oh again, I read too fast, nevermind my comment. |
greg0ire commentedJun 22, 2016
|
greg0ire commentedJun 22, 2016
Link for the lazy :https://travis-ci.org/symfony/symfony/jobs/139483789#L2371 |
d908732 to4fabb70Comparechalasr commentedJun 22, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
For now I removed the FrameworkBundle |
stof commentedJun 22, 2016
@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) |
| { | ||
| $iterator = new \RecursiveIteratorIterator( | ||
| new \RecursiveDirectoryIterator($directory, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::FOLLOW_SYMLINKS), | ||
| \RecursiveIteratorIterator::SELF_FIRST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you should actually useLEAVES_ONLY instead ofSELF_FIRST. You don't need directories in the flattened iterator, only files
| continue; | ||
| } | ||
| $files[] = $file; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
rather than building an array with all files here, we could yield files to iterate over the filesystem little by little
adeacad tof5069f3Comparee24bb86 to5789695Comparechalasr commentedJun 22, 2016
@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: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:Commits-------f54bb16 [TwigBridge] Fix inconsistency in LintCommand help
| } | ||
| if (!$this->isReadable($filename)) { | ||
| throw new \RuntimeException(sprintf('File or directory "%s" is not readable', $filename)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing dot at the end of the message
36d7732 to5129dbeCompareAdd testsFix tests & YamlLintCommand help formattingfabbot fixesUse Generator to iterate over the filesystemMove STDIN related code in a methodUse RecursiveIteratorIterator::LEAVES_ONLY rather than SELF_FIRSTStop using the Finder component when available (Make findFiles() private)Re-add FrameworkBundle YamlLintCommandTestUse CommandTester::getStatusCode() rather than assign execute()Re-add feature for bundle directories, Test it
5129dbe to33402eaComparechalasr commentedJun 23, 2016
I made the changes. Re-added the @BundleName/ feature and a test for. |
fabpot commentedJun 23, 2016
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 usageThis documentssymfony/symfony#19139 (merged on 3.2) andclosessymfony/symfony#19916.Commits-------ed4cb40 Remove an unneeded code directive44b4ad0 Use `terminal` instead of `bash` for console code78075c1 Yaml] Add lint:yaml command usage
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.