Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translation] Added a lint:xliff command for XLIFF files#21578
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
| if (-1 === $error->line) { | ||
| $errorMessages[] = $error->message; | ||
| } else { | ||
| $errorMessages[] = sprintf('Line %d, Column %d: %s', $error->line, $error->column, trim($error->message)); |
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.
in JSON, it may be better to have the line and column as separate properties:{ line: 10, column: 5, message: "unexpected char"}.
It would make the output more useful by allowing to known the location without parsing the error message.
you could do it by building such an array here, and doing the sprintf only indisplayTxt. In this case, I would keep the same structure for general errors, but withnull for the line and column.
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.
Could be interesting to port this change on other lint commands for consistency (yaml, twig)
| */ | ||
| public function isEnabled() | ||
| { | ||
| return class_exists(BaseLintCommand::class) && parent::isEnabled(); |
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 parent logic is justreturn true, so it is not really necessary.
javiereguiluz commentedFeb 10, 2017
Note: I've called this command |
nicolas-grekas commentedFeb 11, 2017
do we have any plan for other formats? if not, I'm not sure about the name. |
phansys left a comment
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.
Status: Needs work.
| $io->text('<error> ERROR </error>'.($info['file'] ? sprintf(' in %s', $info['file']) : '')); | ||
| $io->listing(array_map(function ($error) { | ||
| // general document errors have a '-1' line number | ||
| return -1 === $error['line'] ? $error['message'] : sprintf('Line %d, Column %d: %s', $error['line'], $error['column'], $error['message']); |
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.
If "-1" will be not allowed as line number, "%u" is more suitable as type specifier. The same applies for column.
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.
At the moment, the-1 value is not used ... but this could change at any moment, so let's keep it safe with the%d syntax.
| } | ||
| } | ||
| if ($erroredFiles === 0) { |
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.
Please, use Yoda condition.
Nyholm left a comment
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.
Great. Thank you for this PR. I like it and it sure is needed.
I have briefly reviewed the code and I have run tests on my application.
May I ask you to extract thevalidate function to a separate class? That would make it easier for third party libraries to re-use the validation logic.
javiereguiluz commentedFeb 15, 2017
Thanks for the reviews. About extracting the
So ... I don't know what to do. |
fabpot commentedFeb 18, 2017
Thank you@javiereguiluz. |
Uh oh!
There was an error while loading.Please reload this page.
It works exactly the same as the
lint:yamlcommand:Lint a single file
Lint a bundle
Get the result in JSON