Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for custom ktlint reporters #258

Merged
merged 13 commits into from
Sep 30, 2019

Conversation

Tapchicoma
Copy link
Collaborator

Closes #125

@Tapchicoma Tapchicoma marked this pull request as ready for review July 1, 2019 20:19
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@JLLeitschuh
Copy link
Owner

Can you put a README in samples/kotlin-reporter-creating describing the structure? Would it help?

Do we need to explain to developers that if they side effect change other files outside of the input and output we won't see their changes and it won't get captured in Gradle's "out of date" checking.

Also, just checking, how do custom reporters work with the Gradle Build Cache?

Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
Add test for 3rd party reporters.

Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
@Tapchicoma
Copy link
Collaborator Author

Can you put a README in samples/kotlin-reporter-creating describing the structure? Would it help?

Maybe I misunderstand you, but why we need to add README how to create ktlint reporter? It should be in a ktlint project.

Do we need to explain to developers that if they side effect change other files outside of the input and output we won't see their changes and it won't get captured in Gradle's "out of date" checking.

Can you explain here?

Also, just checking, how do custom reporters work with the Gradle Build Cache?

Should work, as all reporters produces task @OutputFiles property. I will add additional tests for it.

Added test, that checks if reporters are relocatable.

Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
@JLLeitschuh
Copy link
Owner

JLLeitschuh commented Jul 5, 2019

Do we need to explain to developers that if they side effect change other files outside of the input and output we won't see their changes and it won't get captured in Gradle's "out of date" checking.

Can you explain here?

For example:

If you side effect other files in your report, you will need to manually add those side affected files as outputs of the Gradle task. Failure to do so will mean that Gradle will be unable to fully detect all file changed by the task and thus may incorrectly mark your tasks as "up-to-date" when they actually aren't.

@Tapchicoma
Copy link
Collaborator Author

If you side effect other files in your report, you will need to manually add those side affected files as outputs of the Gradle task. Failure to do so will mean that Gradle will be unable to fully detect all file changed by the task and thus may incorrectly mark your tasks as "up-to-date" when they actually aren't.

Sorry, still don't get it fully - how can consume "side-effect" output file?

All 3rd party output reports are added to task outputs, so if they will be removed or modified - task will re-run again.

@JLLeitschuh
Copy link
Owner

JLLeitschuh commented Jul 7, 2019

Example:

class CsvReporter(
    private val out: PrintStream
) : Reporter {
    override fun onLintError(file: String, err: LintError, corrected: Boolean) {
        val line = "$file;${err.line};${err.col};${err.ruleId};${err.detail};$corrected"
        out.println(line)
        File("some_other_file.txt").write(line) // Side effect: `some_other_file.txt` won't be captued as an "output"
    }
}

@Tapchicoma
Copy link
Collaborator Author

Now I got it. Yeah, such "side effect" should be mentioned in README. Not sure if it should be in this project or ktlint? 🤔

@JLLeitschuh
Copy link
Owner

Yeah, such "side effect" should be mentioned in README. Not sure if it should be in this project or ktlint?

Not sure. Both?

@AdamMc331
Copy link
Contributor

Is this still in development? I was looking into the idea of a custom reporter to tweak the colored output for KtLint, being discussed here: pinterest/ktlint#582

Am I unable to implement a custom reporter without this PR being merged?

@Tapchicoma
Copy link
Collaborator Author

Yes, I am planning to finish it before next release (pretty soon). Until then, unfortunately, you can't use custom reporter with this plugin.

@AdamMc331
Copy link
Contributor

Thanks for the update! Ping me if I can provide assistance in any way. :)

Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
To group reporters and support custom one changed DSL to be more nice.

Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
It is obvious from context that file extension is related to reporter.

Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
… state.

Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
@Tapchicoma
Copy link
Collaborator Author

@JLLeitschuh this PR is ready to be reviewed again

Copy link
Owner

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Yahor Berdnikau <egorr.berd@gmail.com>
@Tapchicoma Tapchicoma merged commit 955e8ff into JLLeitschuh:master Sep 30, 2019
@Tapchicoma Tapchicoma deleted the custom-reporters branch September 30, 2019 18:34
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.

Add support for custom reporters.
3 participants