Skip to content

[FEATURE] add codeclimate iteration zero#93

Open
kaystrobach wants to merge 4 commits into
martin-helmich:masterfrom
4viewture:issue-92
Open

[FEATURE] add codeclimate iteration zero#93
kaystrobach wants to merge 4 commits into
martin-helmich:masterfrom
4viewture:issue-92

Conversation

@kaystrobach

Copy link
Copy Markdown

@martin-helmich martin-helmich left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looking good so far! 👍 I've commented a few nitpicks regarding your PR, but nothing major. To top this of, a test case for the new printer (as you've already noted in #92) would be awesome.

Maybe we could also add a "Integrating with Gitlab" section to the README (either within this PR, if you want, or later in a separate change).

Comment thread src/Linter/ReportPrinter/CodeClimateReportPrinter.php Outdated
'type' => 'issue',
'check_name' => $issue->getSource(),
'description' => $issue->getMessage(),
'categories' => ['Style'],

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not sure if all checks fit into the Style category. For instance, the DuplicateAssignmentSniff might also fit into the Bug Risk category.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Do you have a hint how to connect your issue to the categorie? From your code I have not seen an entry point to extract an issue category.

'description' => $issue->getMessage(),
'categories' => ['Style'],
'location' => [
'path' => $file->getFilename(),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not entirely sure -- I think that $file->getFilename() might possibly return the absolute file name, whereas the code climate spec requires the relative file name (to the working dir, presumably).

With #91 (still work-in-progress, however), there is a utility class Helmich\TypoScriptLint\Linter\ReportPrinter\PathUtils coming in that implements just this; maybe we could already pull that class into this PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I know, can you tell me, maybe it makes sense to expose an getAbsolutePath and getRelativePath function from the file object? You could store the pathes splitted in 2 parts and return that easily.

Comment thread src/Linter/ReportPrinter/CodeClimateReportPrinter.php
Co-Authored-By: Martin Helmich <kontakt@martin-helmich.de>
@kaystrobach

Copy link
Copy Markdown
Author

Thanks a lot for your input.

As i said, it was the first itaration for checking if it's possible - and yes it needs some more care :)

@spoonerWeb

Copy link
Copy Markdown

What's the status here? Would love to see this kind of outputs for GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants