-
Notifications
You must be signed in to change notification settings - Fork 121
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
Support annotation processors #93
Support annotation processors #93
Conversation
See discussion in bazeltools#62
("exclude", exportsDoc(ms)) }) | ||
("exclude", exportsDoc(ms)) }, | ||
processorClasses.toList.map { pcs => | ||
("processorClasses", list(pcs.toList.sortBy(_.asString)) { pc => quoteDoc(pc.asString) }) } |
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 do pcs.map(_.asString)
first you can avoid calling it twice, e.g. list(pcs.map(_.asString).toList.sort)(quoteDoc)
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 idea, thanks.
def shortName(fqn: String) = if (fqn.contains(".")) fqn.substring(fqn.lastIndexOf('.') + 1) else fqn | ||
def renderPlugins(pcs: Set[ProcessorClass], exports: Set[Label]): Doc = { | ||
if (pcs.isEmpty) Doc.empty | ||
else processorClasses.toList.map(renderPlugin(pcs, _, exports)).reduce((d1, d2) => d1 + d2) |
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.
you should sort processorClasses
so the output is stable
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.
👍
val fqnToSnakeCase = camel2Underscore(shortName(processorClass.asString)) | ||
s"${name.name}_plugin_$fqnToSnakeCase" | ||
} | ||
def camel2Underscore(text: String) = text.drop(1).foldLeft(text.headOption.map(_.toLower + "") getOrElse "") { |
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 method deserves a unit test
case (acc, c) if c == '$' => acc | ||
case (acc, c) => acc + c | ||
} | ||
def shortName(fqn: String) = if (fqn.contains(".")) fqn.substring(fqn.lastIndexOf('.') + 1) else fqn |
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.
Will the last component always be unique?
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.
Hmm. Potentially you could have v1.Processor
and v2.Processor
in the same java_library
. It seems better then to use the fully-qualified name if there's more than one plugin class to make it simple. In that case, transforming camelCase class names to snake case probably doesn't make sense.
def renderPlugin(pcs: Set[ProcessorClass], pc: ProcessorClass, exports: Set[Label]): Doc = | ||
sortKeys(Doc.text("java_plugin"), getPluginTargetName(pcs, pc), List( | ||
"deps" -> labelList(exports), | ||
"processor_class" -> quote(pc.asString) |
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.
does the plugin need visibility
to match the library?
def getProcessorClasses(u: UnversionedCoordinate): Set[ProcessorClass] = { | ||
val m: Option[Map[ArtifactOrProject, ProjectRecord]] = model.dependencies.toMap.get(u.group) | ||
val projectRecord: Option[ProjectRecord] = m.flatMap(_.get(ArtifactOrProject(u.artifact.asString))) | ||
projectRecord.flatMap(_.processorClasses).getOrElse(Set.empty) |
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.
a for/yield might be cleaner here
- use the fully qualified class name in the `java_plugin` label for uniqueness if there are more than one `annotationProcessors` defined. - sort to `annotationProcessors` by class name to ensure the output is stable - use a for/yield in getProcessorClasses
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 seems great. Made one minor comment but happy to merge soon.
visibility(quote) | ||
)) + Doc.line | ||
|
||
def visibility[T](show: T => Doc): (String, Doc) = |
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.
show is not used is it?
See discussion in #62
I added support for multiple annotation processors per
java_library
, as I think it is a possible (though admittedly rare) requirement.There are a couple TODOs and I would like to add test coverage to the new functions in
Target
but I wanted to start a PR to see if this was directionally correct.