Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Improved the lint:xliff command#26682
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
| return$default($fileOrDirectory); | ||
| } | ||
| privatefunctiongetTargetLanguageFromFile($xliffContents) |
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.
privatefunctiongetTargetLanguageFromFile(\DOMDocument$xliffContents): ?string {foreach ($xliffContents->getElementsByTagName('file')[0]->attributes ??array()as$attribute) {if ('target-language' ===$attribute->nodeName) {return$attribute->nodeValue; } }returnnull; }
| } | ||
| if (false ===$document->schemaValidate(__DIR__.'/../Resources/schemas/xliff-core-1.2-strict.xsd')) { | ||
| $errors[] =array_map(function ($error) { |
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.
are you sure about it ? This will add an array of errors a a single entry in$errors. This looks weird to me
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.
btw, your new test does not seem to cover this code path
| libxml_use_internal_errors(false); | ||
| returnarray('file' =>$file,'valid' =>false,'messages' =>$errorMessages); | ||
| returnarray('file' =>$file,'valid' =>empty($errors),'messages' =>$errors); |
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.
Can you avoid usingempty() here?
This PR was merged into the master branch.Discussion----------Fixed minor issues in the XLIFF filesSpotted thanks to the latest improvement of `lint:xliff`:symfony/symfony#26682Commits-------2020233 Fixed minor issues in the XLIFF files
| * | ||
| * @author Javier Eguiluz <javier.eguiluz@gmail.com> | ||
| */ | ||
| class XliffLintCommandTestextends TestCase |
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.
This test should go in the component, as the command is in the component (well, we may need to have 2 different tests). This should fix the deps=low build too, by having the test for the new feature in the same package than the feature itself.
fabpot commentedMar 30, 2018
Thank you@javiereguiluz. |
This PR was squashed before being merged into the 4.1-dev branch (closes#26682).Discussion----------Improved the lint:xliff command| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#26537| License | MIT| Doc PR | -This is how it looks:Commits-------6bbcc45 Improved the lint:xliff command
This PR was merged into the master branch.Discussion----------Fixed minor issues in the XLIFF filesSpotted thanks to the latest improvement of `lint:xliff`:symfony/symfony#26682Commits-------2020233 Fixed minor issues in the XLIFF files
This PR was merged into the master branch.Discussion----------Fixed minor issues in the XLIFF filesSpotted thanks to the latest improvement of `lint:xliff`:symfony/symfony#26682Commits-------2020233 Fixed minor issues in the XLIFF files
This PR was merged into the master branch.Discussion----------Fixed minor issues in the XLIFF filesSpotted thanks to the latest improvement of `lint:xliff`:symfony/symfony#26682Commits-------2020233 Fixed minor issues in the XLIFF files
This is how it looks: