Conversation
jaschdoc
left a comment
There was a problem hiding this comment.
Looks good! I only have suggestions and comments that might help clarify some paragraphs or add a bit more context.
sarahhaggarty
left a comment
There was a problem hiding this comment.
A good start! I didn't complete a full review yet. The high level structure is OK but I'd like you to break down "Create your own processor" with some smaller headings. Maybe something like:
- Add KSP to your project
- Create an annotation
- Add a symbol processor
- Test the processor
Otherwise the section is too big on it's own. Once you've done this, I can continue reviewing the content :)
ting-yuan
left a comment
There was a problem hiding this comment.
Thanks for the revamp! Just a few nitpicks in the code samples :)
docs/topics/ksp/ksp-quickstart.md
Outdated
| // (1) process() method | ||
| override fun process(resolver: Resolver): List<KSAnnotated> { | ||
| resolver | ||
| .getSymbolsWithAnnotation("MyAnnotation") |
There was a problem hiding this comment.
nit: while this example is correct, can we place MyAnnotation inside a package in step 4, e.g., com.example? This is because getSymbolsWithAnnotation expects fully qualified name (e.g. com.example.MyAnnotation) and less experienced developers might get trapped by mistakenly passing only simple name.
There was a problem hiding this comment.
One more thing along those lines: If the annotation is called MyAnnotation, should the processor and provider then also be named MyAnnotationProcessor and MyAnnotationProcessorProvider?
Edit: My example also has different names, but I will also update that to align the names. https://github.com/google/ksp/pull/2804/changes
There was a problem hiding this comment.
I'll make it HelloWorldAnnotation, so everything has "what it is" as part of its name: HelloWorldAnnotation, HelloWorldProcessor, etc.
docs/topics/ksp/ksp-quickstart.md
Outdated
| 4. Create the following file and declare the annotation `MyAnnotation`: | ||
|
|
||
| ```kotlin | ||
| //processor/src/main/kotlin/MyAnnotation.kt |
There was a problem hiding this comment.
nit: can we move the definition of MyAnnotation out of the processor? I know that this is from the example in the KSP repository, but putting an annotation definition with the processor together is actually an anti-pattern. Most processor libraries have separate artifacts / modules for annotations.
@jaschdoc the example in KSP repo should be updated. Maybe this doc can be updated after the example is updated.
There was a problem hiding this comment.
Which example are you referring to? This one here: google/ksp#2804?
There was a problem hiding this comment.
Hey! I initially tried to have the project organized in that way, with two submodules (one for the processor, one of the annotations), but I couldn't manage to get it to work so decided on this simplified version instead. With three submodules (processor, annotations, app) I had no problems, but at the time I thought that project structure was hard to describe in the quickstart in a straightforward, easy to follow way; I thought minimizing the amount of modules was a good idea.
I now think the three submodule structure is the way to go. That will require some re-writing, but then the result of the quickstart will match the example @jaschdoc and therefore hopefully avoid confusion.
| return codeGenerator.createNewFile( | ||
| dependencies = createDependencyOn(function), | ||
| packageName = "", | ||
| fileName = "GeneratedHelloWorld" |
There was a problem hiding this comment.
Wouldn't this create a copy of GeneratedHellowWorld.kt for each function, and cause FileAlreadyExistsException?
There was a problem hiding this comment.
Correct, but this example assumes that only one symbol will be annotated. I suppose we can use the fully qualified name of the function with a Generated suffix. The same argument can be applied to the generated helloWorld function. If multiple functions are annotated, then (assuming it just appended to the same file) multiple helloWorld functions in the same package are generated.
Using the fully qualified name would solve the issue, but it also adds more complexity to the example. Maybe this is something we handle in the builder example? Maybe we add an aside / warning box to this documentation page after this code snippet that this would crash if multiple functions were annotated.
| </tab> | ||
| </tabs> | ||
|
|
||
| ## Pass options to processors |
There was a problem hiding this comment.
Is this section removed as intended?
There was a problem hiding this comment.
Yes. The information is still present, in the "Create a processor" part. In the future, more pages with more advanced tutorials and examples can be created and this can be expanded upon.
Co-authored-by: Sarah Haggarty <81160244+sarahhaggarty@users.noreply.github.com>
Co-authored-by: Sarah Haggarty <81160244+sarahhaggarty@users.noreply.github.com>
KEDOC-100: New quickstart guide for KSP based on new example in the repo.