-
Notifications
You must be signed in to change notification settings - Fork 29
Add Detekt docs and reorganise existing readme/docs #54
Add Detekt docs and reorganise existing readme/docs #54
Conversation
docs/supported-tools.md
Outdated
with an error like: | ||
|
||
``` | ||
The detekt plugin is configured but not applied. Please apply the plugin in your build script. |
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.
Do we really need to go into this much details? Above paragraph is I think really good but the duplication of the error message is redundant. I don't think this will be helpful to anybody.
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.
Reasoning: you get the error message and search for it. You get a hit, with the resolution steps. Everyone's happy.
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.
Then I would put a link to our docs in the error message. Rather than having it duplicated here.
They would see the error and click the link, that's all. Don't have to search for something.
Duplication is bad because the error may change and it is hard to maintain this copy to be the same.
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.
That's a good idea, tomorrow I will change #53 for that to change the text here and add a link to this bit.
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.
I will postpone this to after I have split out the tools docs, one per file, so I don't have to go back and change things again. I need #57 merged, and then this one merged, to be able to proceed with this.
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.
That sounds good.
docs/supported-tools.md
Outdated
|
||
In order to use Detekt in your project, you need to: | ||
|
||
1. Add this statement to your root `build.gradle` project (change the version according to your needs): |
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.
Actually this is not entirely true. In a simpler project where there is only 1 subproject, you can directly have this there without having anything in root
at all.
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.
Well, not true, because Detekt expects to be applied to the root project, and then to all relevant subprojects.
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.
Either works. You don't have to have it in the root. If you have it in the root, then you don't need to duplicate the version.
My goal around this comment actually was that I think we have too much information here.
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 you have it in the root, then you don't need to duplicate the version.
In the case you were talking about initially, there is only the root, so there is no duplication. In the "normal" case that this guide covers, there is no duplication either since the version is only written in one place, the root build.gradle
.
My goal around this comment actually was that I think we have too much information here.
Do you mean it's bad having the minimum instructions to make the project work? I mean, this is literally the minimum amount of steps required for the Detekt integration to work. If you meant that, as things are, adding Detekt support through SAP is a painful process that requires a lot of explanations to a new starters, I absolutely agree — I maintain that not applying the plugin ourselves makes this UX rather unsatisfactory and it is not fine to leave things as they are in the long term
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.
Let me clarify :) What I mean is that our plugin does not care how Detekt plugin is added. We just expect it to be available.
There are many ways to add a plugin. And can also change in the future. So that's why I thought we can leave it to the official docs. But it is not really that important.
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.
Ok, a bit clearer now :) Well, if your concern is just that we should not mandate how the Detekt plugin is added, we may just want to rephrase the text before the list from:
In order to use Detekt in your project, you need to:
to:
In order to use Detekt, you need to manually add it to **all** your Kotlin projects.
You can refer to the [official documentation](https://github.com/arturbosch/detekt/#gradlegroovy)
for further details. Note that you should _not_ add the `detekt` closure to your
`build.gradle`s, unlike what the official documentation says. The `detekt` closure
in the `staticAnalysis` configuration gets applied to all Kotlin modules automatically.
In most common cases, adding Detekt to a project boils down to three simple steps:
[...]
It is more verbose (mostly because of the not adding detekt
stuff) than just having what we have now, but if you refer to the official docs you must point out not to add the detekt
configuration — lest things not work as expected. That was the reason why, as I explained earlier, I didn't refer users to the official docs (which by the way make a very poor job at explaining what to do).
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.
More verbose but at the same time more maintainable.
I personally like the last one you suggested in above 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.
docs/supported-tools.md
Outdated
For more information see https://github.com/arturbosch/detekt. | ||
``` | ||
|
||
In order to use Detekt in your project, you need to: |
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.
What about having a link to official Detekt documentation? Users will definitely have to go there for details of the configuration anyways. So we would like them to go there and use their docs to learn how to add. And then use our docs to setup.
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.
There is several already, specifically pointing to the relevant bits, where applicable. Our instructions on how to add are different enough from the official ones that they are conflicting, so I chose not to add a direct reference here (but there are in the next sections)
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.
My goal was to keep this docs brief. Considering we will have more tools, having too much information may hurt.
"How to add detekt
" is out of our concern, imho. We expect it to be added, that's all.
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.
I think it would be a grave omission not to have this information here — after all you suggest yourself at line 222 to link to this documentation in the error message.
When we'll add more tools, we'll begin splitting this out into separate docs. I'm already planning that, but it doesn't really make much sense right now imho. I will do it when we add Lint.
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.
After the comment from @mr-archano below asking for a split I put some more thought into the split. I will do it, before we get to Lint, but not in this PR because we'd lose the majority of the comments and discussions. A new PR will follow today with that change.
README.md
Outdated
- convenient way of sharing same setup across different projects | ||
- healthy, versionable and configurable defaults | ||
|
||
### Supported static analysis tools |
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.
Supported tools
to have a shorter title?
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.
Same for me, doesn't really change much ¯\_(ツ)_/¯
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.
README.md
Outdated
Please note that the tools availability depends on the project the plugin is applied to. For more details please refer to the | ||
[supported tools](docs/supported-tools.md) page. | ||
|
||
### Out-of-the-box support for Android and Kotlin projects |
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.
Title includes kotlin
but the content does not mention anything kotlin related.
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.
Not much to mention besides that? I was not sure what to add. Suggestions welcome.
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 we don't, maybe we can remove Kotlin
from this header? Having support for detekt
ktlint
already indicates that we support those tools.
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.
Well, does it not imply it just as much as mentioning Android Lint implies support for Android projects? I don't really see the problem here, to be honest.
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.
I agree with @tasomaniac, imho we could remove Kotlin
from the headline. Additionally we could be a bit more explicit by mentioning PMD
, Findbugs
and Checkstyle
as problematic when it comes to Android Projects.
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.
We have a special paragraph to talk about Android
because the default tools needs setup for Android and does not work out of the box.
That's why this paragraph is really good.
We don't have anything special around Kotlin, imo
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.
@@ -1,23 +1,43 @@ | |||
# gradle-static-analysis-plugin | |||
[![](https://ci.novoda.com/buildStatus/icon?job=gradle-static-analysis-plugin)](https://ci.novoda.com/job/gradle-static-analysis-plugin/lastSuccessfulBuild/console) [![](https://img.shields.io/badge/License-Apache%202.0-lightgrey.svg)](LICENSE.txt) [![Bintray](https://api.bintray.com/packages/novoda/maven/gradle-static-analysis-plugin/images/download.svg) ](https://bintray.com/novoda/maven/gradle-static-analysis-plugin/_latestVersion) | |||
# Gradle static analysis plugin |
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.
What about having a table of contents like section to have links to advanced configuration etc? Because otherwise, I'm afraid people will not see the inlined links.
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.
That wouldn't be a TOC of this document... besides there's extremely little content right now, I don't think it's easy to miss the links
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.
LGTM overall, it's quite some improvement from what we had. I noticed some things were still missing in the readme, I will try to extend this documentation once this PR is merged.
} | ||
``` | ||
|
||
## Checkstyle |
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.
@rock3r what about creating a separate md file for each tool?
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.
As mentioned above, was planning to do it when adding Lint. Can do in this PR if consensus is going that way.
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.
Continues in #54 (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.
good job 👍
just some minor remarks
README.md
Outdated
Please note that the tools availability depends on the project the plugin is applied to. For more details please refer to the | ||
[supported tools](docs/supported-tools.md) page. | ||
|
||
### Out-of-the-box support for Android and Kotlin projects |
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.
I agree with @tasomaniac, imho we could remove Kotlin
from the headline. Additionally we could be a bit more explicit by mentioning PMD
, Findbugs
and Checkstyle
as problematic when it comes to Android Projects.
Groovydocs. | ||
|
||
## Add exclusions with `exclude` filters | ||
You can specify custom patterns to exclude specific files from the static analysis. All you have to do is to specify `exclude` |
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.
This doesn't work for Detekt
as of now. Should we mention that?
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.
Yes. Good spot! I hadn't looked at the Detekt config yet when I checked this :)
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.
docs/advanced-usage.md
Outdated
``` | ||
|
||
## Add exclusions with Android build variants | ||
Sometimes using `exclude` filters could be not enough. When using the plugin in an Android project you may want to consider |
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.
Same as above
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.
Ditto 👍
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.
} | ||
``` | ||
|
||
### Configure Detekt |
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.
Should we add that we suggest to disable thresholds in the detekt
config file?
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.
Good point. Since we cannot automatically do this, it is really important to mention.
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.
Yes! I totally forgot about it!
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.
Tweak Detekt exception message
I have added some explanation on how to exclude files from Detekt. Unfortunately the official docs do not really explain that, so we have to do it ourselves. |
@tasomaniac @mr-archano @tobiasheine all comments addressed. Will start working on splitting out the docs for each tool in their own file next. |
@rock3r Do you wanna do the split as part of this PR or can we merge it? |
Let's just merge it. 👍 |
This PR:
NOTICE
file and reformatting/fixingLICENSE
(wasLICENSE.txt
)docs
folderdocs
folder