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

detekt-generator code examples #584

Merged
merged 7 commits into from
Dec 13, 2017
Merged

detekt-generator code examples #584

merged 7 commits into from
Dec 13, 2017

Conversation

schalkms
Copy link
Member

This PR implements a prototype for compliant and non-compliant code-examples in the documentation generator as discussed in #343.

Do you have an idea where to put the text files with the code examples? @Mauin @arturbosch


import java.io.File

class CodeExampleCollector {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should probably extend Collector so that it can run together with all the other collectors.

Copy link
Member Author

@schalkms schalkms Dec 1, 2017

Choose a reason for hiding this comment

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

Yeah, maybe. The CodeExampleCollector does not need the visit() method.

@@ -23,11 +25,13 @@ class DetektCollector : Collector<RuleSetPage> {
val rules = ruleCollector.items
val multiRules = multiRuleCollector.items.associateBy({ it.name }, { it.rules })
val ruleSets = ruleSetProviderCollector.items
val codeExamples = codeExampleCollector.collect()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be part of the collectors List and then it will run as part of the visit method. codeExampleCollector.items will give you the collected Set.

Copy link
Member Author

Choose a reason for hiding this comment

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

The CodeExampleCollector does not need the visit() method.

val rules: List<RuleCode>
)

data class RuleCode(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we add the codeExample property to Rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

The CodeExample needs to be associated with the corresponding rule.

@@ -42,6 +42,8 @@ inline fun MarkdownContent.referenceToHeading(reference: () -> String) =
"[${reference()}](#${reference().replace(' ', '-')})"

inline fun MarkdownContent.code(code: () -> String) = "`${code()}`"
inline fun MarkdownContent.kotlinCode(code: () -> String) = "```kotlin\n${code()}\n```"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess Markdown would call this a code block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks.

return emptySet()
}

private fun collectExamples(): Set<CodeExample> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where would the examples for compliant/non-compliant code live? Also in the KDoc of a rule? Then we could use a Visitor similar to the RuleCollector. Or should these be completely separate files? In that case it would be hard to link them and to remember to add/update them everytime rules get added/changed because they are so out of sight.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. As I mentioned in the initial comment:

Do you have an idea where to put the text files with the code examples?

I think detekt should have them in separate text files (possibly with the same name as the rule itself).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fear that this "hides" them a little bit and makes very obfuscated where the samples are coming from. Also for Contributors it would not be very obvious that these samples even exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. However, I don't like it in the comment of the rule implementation either.

Copy link
Member

Choose a reason for hiding this comment

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

Didn't we say to put the code examples into the kdoc together with the description?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the rule implementation gets bloated. The code examples don’t provide any additional value to the implementation except for the generated documentation.

Copy link
Member Author

@schalkms schalkms Dec 2, 2017

Choose a reason for hiding this comment

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

@arturbosch that’s the advantage of the inline solution

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hiding code examples makes the codebase harder understand and fully comprehend imo. It will be really easy to forget adding/keeping those examples up-to-date when they are so out of sight. Having everything in one place in the KDoc of the rule class at least makes it a little more visible and more likely that contributors and the people reviewing PRs remember that the examples should be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

This could be avoided by a simple test as I mentioned.
Anyways, the votes are against this solution. I can of course live with that. I’ll adapt this prototype soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the open discussion and the review!

Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

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

I like the <compliant> pattern for the examples, better to read than @compliant with indentation 👍

}
```

#### Compliant Code:
Copy link
Member

Choose a reason for hiding this comment

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

I think the Compliant and Noncompliant code snippets are mixed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. I will update it.

@schalkms schalkms removed the blocked label Dec 11, 2017
@arturbosch arturbosch merged commit a1c192f into detekt:master Dec 13, 2017
@schalkms schalkms deleted the detekt-generator-code-examples branch December 13, 2017 19:38
@arturbosch arturbosch added this to the RC6 milestone Dec 20, 2017
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