-
Notifications
You must be signed in to change notification settings - Fork 2
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
Emit events for all known types #127
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #127 +/- ##
============================================
+ Coverage 79.35% 81.73% +2.38%
- Complexity 236 240 +4
============================================
Files 73 79 +6
Lines 1889 1993 +104
Branches 140 146 +6
============================================
+ Hits 1499 1629 +130
+ Misses 335 313 -22
+ Partials 55 51 -4 |
@armiol, @alexander-yevsyukov, PTAL. This is a small yet consequential change set. Let's further discuss it a video call if necessary. |
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.
Please see my comments. In general, I think that the generation_requested
flag approach should be discussed.
report_paths: '**/build/test-results/test/TEST-*.xml' | ||
require_tests: true # will fail workflow if test reports not found | ||
|
||
# See: https://github.com/marketplace/actions/junit-report-action |
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.
Can we revert these changes please? Separation of test reports was needed some time ago when investigating build failures at CI.
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.
@dmdashenkov I think this comment still stands.
@@ -68,13 +68,15 @@ internal class EnumCompilerEvents( | |||
EnumEntered.newBuilder() |
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.
Please consider using Kotlin DSL for Protobuf. It should be already available here. I've just tried it in another branch and it worked for me.
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.
@dmdashenkov I am now requesting changes, so that you could re-request from me, once this PR is ready. Alexander is off for a while, so it will be just me.
…s produce the same output
@armiol, PTAL. |
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.
@dmdashenkov please see my comments.
report_paths: '**/build/test-results/test/TEST-*.xml' | ||
require_tests: true # will fail workflow if test reports not found | ||
|
||
# See: https://github.com/marketplace/actions/junit-report-action |
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.
@dmdashenkov I think this comment still stands.
@@ -264,3 +265,15 @@ message RpcExited { | |||
|
|||
RpcName rpc = 3; | |||
} | |||
|
|||
// Emitted when a Protobuf dependency file is discovered. |
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.
Is this about discovering an import
in our code? I am not sure what do you mean by "discovered".
// Normally, no code should be generated for dependencies. However, they can be used for additional | ||
// info when generating code for the module's own types. | ||
// | ||
message DependencyDiscovered { |
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 would be more precise with this name, so that Java- and Proto-level dependencies could be distinguished.
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've updated the doc somewhat. I'd like to avoid adding the Proto
prefix because:
- we only work with Protobuf, no Java or JS or other dependency is ever considered in ProtoData (not talking about the Gradle compiler here, only ProtoData proper);
- this is one of a series of events such as
TypeEntered
,OptionDiscovered
, etc., which do not specify the language either.
} | ||
|
||
/** | ||
* Assigns the field type and cardinality (`map`/`list`/`oneof_name`/`single`) to the receiver |
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.
Is this, in fact, copying the field type and the cardinality? If so, maybe we could get rid of this "assign" term which feels a bit over-used in our domain?
* Assigns the field type and cardinality (`map`/`list`/`oneof_name`/`single`) to the receiver | ||
* builder. | ||
* | ||
* @return the receiver for method chaining. |
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, in Kotlin we still don't put periods in @return
statement descriptions.
/** | ||
* Produces a sequence by walking through all the nested message definitions staring with `type`. | ||
* | ||
* @param type the message definition which may contain nested message definition to walk though |
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.
"through"
* | ||
* The event reflects all the definitions from the file. | ||
*/ | ||
internal fun toDependencyEvent(fileDescriptor: Descriptors.FileDescriptor) = |
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.
How about discoverDependencies(fileDescriptor: Descriptors.FileDescriptor)
?
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.
How about discoverDependencies(fd: FileDescriptor)
? :)
import com.google.protobuf.Descriptors.MethodDescriptor | ||
import com.google.protobuf.Descriptors.OneofDescriptor | ||
import com.google.protobuf.Descriptors.ServiceDescriptor | ||
import io.spine.protodata.Doc | ||
import io.spine.string.trimWhitespace | ||
import io.spine.util.interlaced | ||
|
||
/** | ||
* Documentation contained in a Protobuf 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.
Given its description, I don't really understand why such a generic thing is now in .backend
.
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'm not a fan of the word .backend
either.
This is the top-level package name for the :compiler
module. :compiler
serves as main implementation module for ProtoData.
Previously, Documentation
, CompilerEvents
, and other types where placed in the :api
module in ...event
package. However, it's not clear that those components belong in the :api
module. Moving them to :compiler
is logical since this is clearly an implementation detail (hence the internal
modifier). The change of the package is a mere consequence.
I could repackage said components into ...backend.event
, but I don't see much value in that right now. Would love to hear what you think.
@@ -149,10 +154,10 @@ private constructor(private val value: List<Int>) { | |||
*/ | |||
fun fromMessage(descriptor: Descriptor): LocationPath { | |||
val numbers = mutableListOf<Int>() | |||
numbers.add(FileDescriptorProto.MESSAGE_TYPE_FIELD_NUMBER) | |||
numbers.add(DescriptorProtos.FileDescriptorProto.MESSAGE_TYPE_FIELD_NUMBER) |
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 a lot of such changes in this PR. Don't you think it's less convenient to address the types and their members by specifying a "full" (so to speak) name, rather than stripping some common parts by having them "statically" imported?
import io.spine.protodata.OneofGroup | ||
import io.spine.protodata.TypeName | ||
import io.spine.protodata.event.FieldEntered |
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.
Given that the events apparently are still in io.spine.protodata.event
package, I don't understand why MessageCompilerEvents
isn't.
I have the same question regarding all the similar cases. Maybe, we should discuss the idea of re-packaging things into .backend
as a whole. Because right now I don't understand the goal.
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.
@dmdashenkov, we do need event
package so that Javadocs, KDocs, and source code JARs are easier to understand. Please do have events coming under the event
package.
* A type URL contains the type URL prefix and the qualified name of the type separated by | ||
* the slash (`/`) symbol. See the docs of `google.protobuf.Any.type_url` for more info. | ||
*/ | ||
public fun typeUrl(): String? { |
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 the return value can really be null
, please document when such a case can occur.
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.
ProtoDeclarationName
has a similar method that returns non-nullable String
. How these two types are different in that regards?
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.
Oops, that was a mistake. Fixed it.
doc = documentation.forField(this@buildField) | ||
[email protected] = [email protected] | ||
[email protected] = declaringType | ||
copyTypeAndCardinality(this@buildField) |
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.
Please extract this@buildField
into a variable (e.g. fieldDescr
, or thisField
). This way you'd have fewer “dogs” around.
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.
Please see my additional comments.
declaredIn = declaringType | ||
[email protected] = [email protected] | ||
orderOfDeclaration = index | ||
doc = documentation.forEnumConstant(this@buildConstant) |
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.
Similarly to the comment above, extracting this@buildConstant
into a val
in this block could improve the readability.
compiler/src/main/kotlin/io/spine/protodata/backend/ServiceCompilerEvents.kt
Outdated
Show resolved
Hide resolved
@alexander-yevsyukov, @armiol, PTAL again. |
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.
Please see my comments. Almost there.
* A type URL contains the type URL prefix and the qualified name of the type separated by | ||
* the slash (`/`) symbol. See the docs of `google.protobuf.Any.type_url` for more info. | ||
*/ | ||
public fun typeUrl(): String { |
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.
Why don't we have it as another property rather than a function? Just curious, how you see it.
One reason I can think of is to have shorter names in Java code. But we aim to Kotlin anyway, and we always can have @JvmName
defined, should we want to.
@@ -183,8 +201,8 @@ class CompilerEventsSpec { | |||
|
|||
event.option.name shouldBe "idempotency_level" | |||
event.option.value.unpackGuessingType() shouldBe enumValue { | |||
name = IdempotencyLevel.NO_SIDE_EFFECTS.name | |||
number = IdempotencyLevel.NO_SIDE_EFFECTS_VALUE | |||
name = DescriptorProtos.MethodOptions.IdempotencyLevel.NO_SIDE_EFFECTS.name |
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.
Can we import MethodOptions
, please?
@alexander-yevsyukov, PTAL again. |
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
Since we've discussed this PR in a call already.
In this PR we enable ProtoData plugin authors to listen to Protobuf compiler events that describe types from dependencies.
Previously, events like
TypeEntered
were only emitted for Proto definitions from within the module that ProtoData is processing. This is good for many cases, since this removes the need to check whether or not any code has to be generated based on a given event. However, this limited the users to only the types defined in the module. There was no way to know about a Proto message type from outside the module.Now, we introduce a new
DepdendencyDiscovered
event and aProtobufDependencyFile
view. The two new types allow ProtoData plugin authors to generate code with awareness of the module's dependencies.