feat: introduce verb visibility modifiers #1326
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Open
wesbillman
force-pushed
the
verb-visibility
branch
from
29c78f4 to
e27112e
Compare
wesbillman
force-pushed
the
verb-visibility
branch
from
e27112e to
8b383ae
Compare
worstell
approved these changes
Apr 25, 2024
wesbillman
force-pushed
the
verb-visibility
branch
from
8b383ae to
fbde13a
Compare
wesbillman
force-pushed
the
verb-visibility
branch
from
fbde13a to
d8a7858
Compare
worstell
reviewed
Apr 25, 2024
| val path: String? | ||
| ) | ||
|
|
||
| private fun extractExportAnnotation(verb: KtNamedFunction, bindingContext: BindingContext): ExportAnnotation { |
There was a problem hiding this comment.
nit- i think this can be accomplished without any member data and so it can be a static function declared in the companion object below, like the following:
private fun KtAnnotationEntry.toExportAnnotation(): ExportAnnotation {
var visibility: Visibility? = null
var ingress: Ingress? = null
var method: Method? = null
var path: String? = null
exportAnnotation?.valueArguments?.forEach { arg ->
when {
arg.getArgumentName()?.asName?.asString() == "visibility" || arg.getArgumentExpression()?.getType(bindingContext)?.fqNameOrNull()?.asString() == Visibility::class.qualifiedName -> {
visibility = kotlin.runCatching { Visibility.valueOf(arg.getArgumentExpression()?.text?.substringAfterLast('.') ?: "") }.getOrNull()
}
arg.getArgumentName()?.asName?.asString() == "ingress" || arg.getArgumentExpression()?.getType(bindingContext)?.fqNameOrNull()?.asString() == Ingress::class.qualifiedName -> {
ingress = kotlin.runCatching { Ingress.valueOf(arg.getArgumentExpression()?.text?.substringAfterLast('.') ?: "") }.getOrNull()
}
arg.getArgumentName()?.asName?.asString() == "method" || arg.getArgumentExpression()?.getType(bindingContext)?.fqNameOrNull()?.asString() == Method::class.qualifiedName -> {
method = kotlin.runCatching { Method.valueOf(arg.getArgumentExpression()?.text?.substringAfterLast('.') ?: "") }.getOrNull()
}
arg.getArgumentName()?.asName?.asString() == "path" || arg.getArgumentExpression()?.getType(bindingContext)?.fqNameOrNull()?.asString() == String::class.qualifiedName -> {
path = arg.getArgumentExpression()?.text?.trim('"')
}
}
}
return ExportAnnotation(visibility, ingress, method, path)
}
then to use it, you'd do annotationEntry.toExportAnnotation()
worstell
reviewed
Apr 25, 2024
| return verb.annotationEntries.firstOrNull { | ||
| bindingContext.get(BindingContext.ANNOTATION, it)?.fqName?.asString() == HttpIngress::class.qualifiedName | ||
| bindingContext.get(BindingContext.ANNOTATION, it)?.fqName?.asString() == Export::class.qualifiedName |
There was a problem hiding this comment.
since we get the ingress information from the export annotation now, how about we pass the full KtAnnotationEntry instead of ExportAnnotation? then we won't need to derive it again and we can lift everything out of the let below. we can use annotationEntry.toExportAnnotation() to get the export fields
alecthomas
approved these changes
Apr 25, 2024
| Request: &schema.Ref{Name: "EchoRequest"}, | ||
| Response: &schema.Ref{Name: "EchoResponse"}, | ||
| Name: "echo", | ||
| Visibility: "internal", |
There was a problem hiding this comment.
Use the constants here and elsewhere rather than literals - it will result in a compiler error if they get renamed, unlike the literals which will silently add invalid data.
|
|
||
| Export bool `parser:"@'export'"` | ||
| Visibility schema.Visibility `parser:"@('public' | 'internal' | 'private')"` |
There was a problem hiding this comment.
Can we somehow catch //ftl:export (probably not here) and return an error indicating to use these now?
| isVerb = true | ||
| if pctx.module.Name == "" { | ||
| pctx.module.Name = pctx.pkg.Name | ||
| } else if pctx.module.Name != pctx.pkg.Name { | ||
| pctx.errors.add(errorf(node, "function export directive must be in the module package")) | ||
| pctx.errors.add(errorf(node, "function visibility directive must be in the module package")) |
There was a problem hiding this comment.
What does this error mean exctly? Must be in the root module package?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1324
This adds the new schema changes and parser implementations. Once this PR lands, we'll need to update all the other repos to make sure they work with these breaking changes.