Fix filtering #173

dmdashenkov · 2023-09-08T14:01:17Z

In this PR we introduce a better way for plugin developers to control the sources they are working with.

For this, a new term is introduced — source file set label (a.k.a. label). A label consists of the programming language used by the files in the file set and the name of the code generator that created the files. The label is declarative, i.e. the user who configures ProtoData is responsible for providing accurate info.

Typically, a label corresponds to one protoc plugin/builtin. However, protoc treats all the outputs of all plugins/builtins equally, disregarding their relations (e.g. using the same language, etc.). Thus, we require the extra details to be configured by the ProtoData user.

Now, a Renderer may choose which source file sets it wants to process. For this, the implementations should override the supports(SourceFileSetLabel) method. Only the supported source file sets will be passed to render(..). For example, if a renderer only needs to process Java gRPC stubs, it would run the following check:

public class MyRenderer extends JavaRenderer {

    @Override
    public boolean supports(SourceFileSetLabel label) {
        return label.getLanguage().equals(Java.lang()) && 
               label.getGenerator().getName().equals("grpc"); 
    }

    // ...
}

New CLI and Gradle APIs are added for specifying the labels.

In CLI, the source file sets are now supplied by a single parameter --paths. The parameter consists of the label (including the language and the code generator name), the source path, and the target path. Legacy --source-root and --target-root params are no longer supported. See PathsParam help message for more info.

In Gradle DSL, the source file sets are supplied via the SourcePaths objects. The preferred way of creating them is as follows:

protodata {

    launchFor(sourceSets.main) {
         sourceFileSet {
            source = "$buildDir/generated-proto/main/java"
            target = "$buildDir/footest-java"
            language = "java"
        }
        sourceFileSet {
            source = "$buildDir/generated-proto/main/grpc"
            target = "$buildDir/footest-java-grpc"
            language = "java"
            generatorName = "grpc"
        }
    }

    launchFor(sourceSets.test) {
         sourceFileSet {
            source = "$buildDir/generated-proto/test/java"
            target = "$buildDir/footest-test-java"
            language = "java"
        }
    }
}

The protodata.paths property provides a more file-grained control over the registered source file sets.

Fixes SpineEventEngine/compiler#23, fixes SpineEventEngine/compiler#17.

# Conflicts: # license-report.md # pom.xml # version.gradle.kts

codecov · 2023-09-20T08:07:43Z

Codecov Report

Merging #173 (9a3158e) into master (2bc2b03) will decrease coverage by 2.17%.
The diff coverage is 38.50%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     SpineEventEngine/ProtoData#173      +/-   ##
============================================
- Coverage     82.11%   79.94%   -2.17%     
+ Complexity      364      363       -1     
============================================
  Files           104      114      +10     
  Lines          2600     2688      +88     
  Branches        188      192       +4     
============================================
+ Hits           2135     2149      +14     
- Misses          385      462      +77     
+ Partials         80       77       -3

dmdashenkov · 2023-09-20T14:54:31Z

@alexander-yevsyukov, PTAL. @armiol, PTAL.

armiol

@dmdashenkov please see my comments.

armiol · 2023-09-21T12:42:03Z

api/src/main/kotlin/io/spine/protodata/renderer/SourceFileSetLabel.kt

+/**
+ * A label for a source file set.
+ *
+ * The label marks the programming language that the files use and the name of the generator that


With the usage example (the one from the PR description) it is not clear how labels relate to language and generatorName. More than that, with this description, it is now more confusing.

I would use the same terms both in CLI, Gradle DSL, and in the code.

armiol · 2023-09-21T12:48:22Z

api/src/main/kotlin/io/spine/protodata/renderer/SourceGeneratorName.kt

+/**
+ * A name of a custom source code generator.
+ *
+ * May represent a Protobuf compiler plugin, or any other code generator.


This is something strange.

"Generator" in all cases is the thing producing the code. Why would we need a "default" and "custom" generators? They both generate something, and why would we care about their origin?

And again, in this piece of documentation, you mention "labels". Which still does not add up: "generator"s, source set labels and "language" all dance around :(

"Default" simply references the case most users will likely face most of the time. And it is the default way in which Protoc generated code, as opposed to custom generators/Protoc plugins.

I've cleared up the doc's wording a bit so that there is no confusion with the labels.

armiol · 2023-09-21T12:50:35Z

cli-api/src/main/kotlin/io/spine/protodata/cli/CommandLineInterface.kt

+          2. The source path. Must be an absolute path where the code files are located. If there
+             are no code files, i.e. ProtoData should generate all the code from scratch, this part
+             may be left blank. 
+          2. The target path. Must be an absolute path where the code files should be placed after


armiol · 2023-09-21T12:52:03Z

cli-api/src/main/kotlin/io/spine/protodata/cli/KnownLanguages.kt

+    Java,
+    Kotlin,
+    JavaScript,
+    AnyLanguage


This is weird in terms of allowing to use this value in CLI.

armiol · 2023-09-21T12:54:09Z

gradle-api/src/main/kotlin/io/spine/protodata/gradle/CodegenSettings.kt

+    /**
+     * `SourcePaths` to run ProtoData on.
+     *
+     * The keys to the multimap are the scopes, i.e. Gradle's source set names,


This String-typed "scope" sounds like a "label" to me. Or like a "generator". I.e., more confusing than not.

If this is a source set name, let's say so.

Same below.

armiol · 2023-09-21T12:56:16Z

gradle-api/src/main/kotlin/io/spine/protodata/gradle/SourcePaths.kt

+        require(!language.isNullOrBlank()) { missingMessage("language") }
+    }
+
+    private fun missingMessage(propName: String) = "Source file set requires the $propName."


Let's separate $propName with some symbols, such as ticks or back-ticks.

armiol · 2023-09-21T13:01:14Z

gradle-plugin/src/main/kotlin/io/spine/protodata/gradle/plugin/Extension.kt

+     * from the present data for backward compatibility. However, in this compatibility mode,
+     * only Java and Kotlin source file sets can be constructed.
+     */
+    internal fun pathsOrCompat(sourceSet: SourceSet): Set<SourcePaths> {


The problem here is that we need to remember which version (this one or paths) to use. And there is no clear sign on when to stop using one and start using another.

Maybe, we could have a "compatibility" mode turned on always. And for testing (or whatever we need "strictly new API" mode for) have another method, which would ignore the deprecated properties?

I've added a note on this in doc. Essentially, this method needs to exist until mc-java is updated. Potentially, until the very next PR. Then, only paths need to be used.

armiol · 2023-09-21T13:02:21Z

gradle-plugin/src/main/kotlin/io/spine/protodata/gradle/plugin/Extension.kt

+        return srcRoots.zip(targetRoots)
+            .map { (src, target) ->
+                val pathSuffix = src.asFile.toPath().name
+                val lang = if (pathSuffix == "kotlin") Kotlin else Java


Instead of using a literal, maybe we could use knownLanguages, which we have previously created exactly to associate Kotlin with "kotlin"?

armiol · 2023-09-21T13:03:30Z

gradle-plugin/src/main/kotlin/io/spine/protodata/gradle/plugin/Extension.kt

+         * @see subDirs
+         */
+        public val defaultSubdirectories: List<String> = listOf(
+            "java",


With all these words constantly repeating, it again strikes me on why we don't re-use the definitions, say, from either languages, or generators (or labels), that we know exist.

Some here. This list will die off once the old subdirs API is removed.

armiol · 2023-09-21T13:03:56Z

gradle-plugin/src/main/kotlin/io/spine/protodata/gradle/plugin/Extension.kt

+            "grpc",
+            "js",
+            "dart",
+            "spine",


I think we don't generate anything into "spine" for a while already.

dmdashenkov · 2023-09-21T15:35:33Z

@armiol, PTAL again.

alexander-yevsyukov

I don't understand why we need extra settings for ProtoData while we know what Protobuf and its Gradle Plugin have for languages and generators, like gRPC.

It's either a plugin or built-in in Protobuf codegen terms. The set of supported builtins is known. There are also known plugins like gRPC.

We can get this information from there and transform into Language and Generator terms.

alexander-yevsyukov · 2023-09-21T16:49:36Z

We can get this information from there and transform into Language and Generator terms.

When I wrote this I meant we don't need yet another term "label".

armiol

@dmdashenkov basically, the gist of this PR is as follows.

We could have discussed the approach beforehand, or deal with a number of newly introduced language terms in scope of PR. The latter is longer, but here we are, and there is not much time left to make things conclude with a resolution leaving all parties happy.

I suggest we gather tomorrow on a call with @alexander-yevsyukov and discuss these changes again, instead of bouncing thing back and forth in a PR.

dmdashenkov · 2023-09-22T15:18:19Z

@armiol, @alexander-yevsyukov, PTAL at the current state of this changeset. I've reflected the main points of our discussion in the PR header and changed some of the naming accordingly.

dmdashenkov added 3 commits September 7, 2023 13:14

Bump version 0.12.0

5eae93f

Introduce SourceFileSetMarker

d140c06

Add stub markers

91a084b

dmdashenkov self-assigned this Sep 8, 2023

dmdashenkov added 10 commits September 11, 2023 11:58

Simplify a precondition check

368b73e

Reduce repetition in tests

436df68

Use paths in the Gradle plugin (WIP)

e7bea69

Fix CLI param syntax

162cc35

Fix alignment

068c938

Check if a renderer supports the given sources file set

5204e0c

Make regex parsing more resilient

4bcdce0

Merge branch 'master' into fix-filtering

162142f

# Conflicts: # license-report.md # pom.xml # version.gradle.kts

Update Gradle plugin tests

1dd7385

Set up incremental build

186e1c2

dmdashenkov added 8 commits September 20, 2023 11:12

Add some doc

0fcecf9

More doc

368ae6e

More doc

1c32f45

More doc and stricter deprecation

454ca48

Yet more doc

6d69dbc

More doc for test env

5ddd630

Remove debug output

2793061

Remove redundant checks, add logging and another test

dffa31e

dmdashenkov marked this pull request as ready for review September 20, 2023 14:54

dmdashenkov requested review from alexander-yevsyukov and armiol September 20, 2023 14:54

armiol requested changes Sep 21, 2023

View reviewed changes

dmdashenkov added 2 commits September 21, 2023 17:10

Remove unwanted generated subdirs

8cf7162

Fix numbering

0742a7e

dmdashenkov added 6 commits September 21, 2023 17:26

Better doc

2370ae3

More doc

484e0d3

Better error message

8747165

Remove a duplicate string literal

09ccc19

More doc

b006441

Make language reference for AnyLanguage simpler

44c297d

dmdashenkov requested a review from armiol September 21, 2023 15:35

alexander-yevsyukov requested changes Sep 21, 2023

View reviewed changes

armiol requested changes Sep 21, 2023

View reviewed changes

dmdashenkov added 2 commits September 22, 2023 17:54

Rename new DSL components

54bcd5a

Add a shortcut for Gradle's providers

9a3158e

dmdashenkov removed their assignment Sep 22, 2023

Fix filtering #173

Are you sure you want to change the base?

Fix filtering #173

Uh oh!

Conversation

dmdashenkov commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmdashenkov commented Sep 20, 2023

Uh oh!

armiol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmdashenkov Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmdashenkov commented Sep 21, 2023

Uh oh!

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-yevsyukov commented Sep 21, 2023

Uh oh!

armiol left a comment

Choose a reason for hiding this comment

Uh oh!

dmdashenkov commented Sep 22, 2023

Uh oh!

Uh oh!

dmdashenkov commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 20, 2023 •

edited

Loading

dmdashenkov Sep 21, 2023 •

edited

Loading