Pre commit hooks and Samples update #86
Conversation
.pre-commit-config.yaml
Outdated
| hooks: | ||
| - id: detect-secrets | ||
| args: [--baseline, .secrets.baseline] | ||
|
|
||
| # Do NOT run pre-commit hooks on generated-code | ||
| exclude: ^samples/[^/]+/src/main/java |
There was a problem hiding this comment.
Are we sure about this?
KtLint was sanitising the generated code a lot. Now it doesn't really look that elegant as before. Any arguments for having this exclude rule here?
There was a problem hiding this comment.
The proposal here, and I'm more than happy to remove this line if needed, is that samples should contain the exact output of the tool.
If the generated code does not look "elegant" then we should eventually update the tool to generate some more elegant result rather than having ktlint fixing it.
One of the many things that I was noticing is that we have import okhttp3.RequestBody on all *Api.kt files even if we don't have parameters of type: file.
There was a problem hiding this comment.
If the generated code does not look "elegant" then we should eventually update the tool to generate some more elegant result rather than having ktlint fixing it.
This is not always possible due to limitations of mustache.
I'm generally fine with:
generator outputting not elegant code -> formatter fixing it -> eventually committing it.
There was a problem hiding this comment.
What might eventually be a concern is that having the formatter fixing the file might hide issues within the generated code.
Think about the import okhttp3.RequestBody. The linter removes it because RequestBody is not used in most of the generated files. That's great, but what if we are using the generated code into a pipeline that generates an artifact? Probably we're going to have issues because the import statement (with the typo) will fail or similar.
About mustache limitations, I agree, I'll try to work on it to understand how complex would be to generate "elegant" enough code.
There was a problem hiding this comment.
Keeping this open as we're going to move the effort to address this specific comment into a separate PR
|
In order to get the pre-commit hooks and samples updated I'm removing from this PR the pre-commit exclusion of the samples. I'll address #86 (comment) into a subsequent PR (the new PR will mostly focus on having mustache generate a "nicer" code that will make ktlint a bit happier). |
macisamuele
force-pushed
the
maci-pre-commit-hooks-and-samples-update
branch
from
7a503e7
to
4a9fbf9
Compare
…d value toward teastability or showing how the generated code looks like
macisamuele
force-pushed
the
maci-pre-commit-hooks-and-samples-update
branch
from
4a9fbf9
to
5f83124
Compare
Please update the description of this PR |
…-and-samples-update
As mentioned in #82 this follow-up PR ensures: