Skip to content

Adding linting for the new add-lines configurator#14

Merged
fabpot merged 1 commit intosymfony-tools:mainfrom
weaverryan:add-lines-linting
May 26, 2023
Merged

Adding linting for the new add-lines configurator#14
fabpot merged 1 commit intosymfony-tools:mainfrom
weaverryan:add-lines-linting

Conversation

@weaverryan
Copy link
Copy Markdown
Contributor

Supports symfony/flex#975

Comment thread src/LintManifestsCommand.php Outdated
Comment thread src/LintManifestsCommand.php Outdated
@weaverryan
Copy link
Copy Markdown
Contributor Author

Good suggestions - I implemented both and tested it locally.

Comment thread src/LintManifestsCommand.php Outdated
Comment thread src/LintManifestsCommand.php Outdated
}

if (!is_string($addLine[$key])) {
$output->writeln(sprintf('::error file=%s::"add-lines" has a "%s" key but it must be a string value', $manifest, $key));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe those error messages should indicate for which item of $data they are, given that we are not able to associate the error with a precise line in the file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. It's a bit tricky to know WHAT to show - so I included the index integer of where this is inside of add-lines.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think the index is fine.

@fabpot fabpot force-pushed the add-lines-linting branch from c51cc0b to 89b84e9 Compare May 26, 2023 16:29
@fabpot
Copy link
Copy Markdown
Contributor

fabpot commented May 26, 2023

Thank you @weaverryan.

@fabpot fabpot merged commit 8ec0214 into symfony-tools:main May 26, 2023
@weaverryan weaverryan deleted the add-lines-linting branch May 26, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants