Generate a method providing `TypeUrl` for Protobuf types compiled to JS #255

dmytro-grankin · 2018-12-17T13:31:56Z

A possibility to obtain a TypeUrl for a message is convenient, but Protobuf definitions compiled to JS don't provide such a possibility.

So, to allow convenient obtaining of TypeUrl, this PR enhances ProtoJsPlugin. The plugin now generates the static typeUrl() method for every message and enum type.

Other changes:

The naming of classes related to JS code-generation was improved.
The package structure for ProtoJsPlugin was changed to keep related classes in a single package.
A code Snippet was introduced as a replacement of JsCodeGenerator. The Snippet has no side effects as the generator does.
CodeLine was made abstract and now have different implementations.
The configuration of test projects via GradleProject was changed. Now, instead of copying config submodule and version.gradle, the path to the root of spine-base is supplied. So, test configuration can reference e.g. the version.gradle via ${enclosingRootDir}/version.gradle.

The further improvements is a matter of SpineEventEngine/mc-js#8.

codecov · 2018-12-17T15:26:51Z

Codecov Report

Merging #255 into master will increase coverage by 0.05%.
The diff coverage is 87.63%.

@@             Coverage Diff              @@
##             master     SpineEventEngine/base#255      +/-   ##
============================================
+ Coverage     74.68%   74.74%   +0.05%     
- Complexity      649      689      +40     
============================================
  Files           292      302      +10     
  Lines          8051     8236     +185     
  Branches        537      551      +14     
============================================
+ Hits           6013     6156     +143     
- Misses         1911     1944      +33     
- Partials        127      136       +9

dmytro-grankin · 2018-12-18T08:36:53Z

@dmdashenkov PTAL.

dmdashenkov

@dmytro-grankin, please see my comments.
As a general notice, some classes seem a bit ad hoc, e.g. when using raw String type to represent a piece of JS code. Maybe, there is a layer of abstraction missing.

dmdashenkov · 2018-12-18T09:28:48Z

base/src/main/java/io/spine/code/proto/EnumType.java

@@ -46,7 +47,8 @@ private EnumType(EnumDescriptor descriptor,
        super(descriptor, descriptorProto, className, typeUrl);
    }

-    private static EnumType create(EnumDescriptor descriptor) {
+    @VisibleForTesting
+    public static EnumType create(EnumDescriptor descriptor) {


Please find a way to make this method package-private. public is way too broad.

dmdashenkov · 2018-12-18T09:29:17Z

base/src/main/java/io/spine/code/proto/MessageType.java

+     * Creates a message type for the provided descriptor.
+     */
+    @VisibleForTesting
+    public static MessageType create(Descriptor descriptor) {


Same as above.

dmdashenkov · 2018-12-18T09:30:47Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/FileSetEnhancement.java

+     * <p>Does nothing if there are no Protobuf files to process.
+     */
+    public final void perform() {
+        if (!hasFilesToProcess()) {


Please (un-)invert the condition. This way the code becomes a bit more descriptive.

dmdashenkov · 2018-12-18T09:39:08Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/JsType.java

+     * Obtains the name of the type in the generated code.
+     */
+    public TypeName name() {
+        return TypeName.from(type.descriptor());


There is the Type.name() method for this.

It's a different TypeName, from io.spine.code.js.

dmdashenkov · 2018-12-18T09:44:59Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/parse/TypesParsingExtension.java

@@ -37,13 +36,13 @@
 * <p>The class generates the {@code fromJson} and {@code fromObject} methods for each message
 * declared in the {@link FileDescriptor}.
 */
-public final class FileGenerator extends JsCodeGenerator {
+public final class TypesParsingExtension extends JsCodeGenerator {


Suggested change

public final class TypesParsingExtension extends JsCodeGenerator {

public final class TypeParsingExtension extends JsCodeGenerator {

dmdashenkov · 2018-12-18T09:51:02Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/typeurl/TypeUrlMethod.java

+    @Override
+    public void generate() {
+        generateStaticMethod();
+        jsOutput().addEmptyLine();


The TypeUrlMethod class, apparently, as well as all JsCodeGenerators use an output parameter jsOutput instead of returning values. Such a pattern adds a side effect to the JsCodeGenerator.generate() method.

Instead, we should make the method pure and write the result to the output when necessary.

dmdashenkov · 2018-12-18T09:54:29Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/FileSetEnhancement.java

+/**
+ * An enhancement of generated Protobuf sources.
+ */
+public abstract class FileSetEnhancement {


Please elaborate on the class description. Maybe, you'll come up with a better name while doing so. Currently, I do not entirely understand the concept.

dmdashenkov · 2018-12-18T09:56:27Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/parse/ParsingOfObjects.java

@@ -60,57 +58,34 @@
 *         message stored in a file.
 * </ol>
 */
-public final class JsonParsersWriter {
+public final class ParsingOfObjects extends FileSetEnhancement {


This class name is strange. From the name, it seems like the object describes the process of parsing.
Please think of a better name.

outside of the package

dmytro-grankin · 2018-12-26T10:40:14Z

@armiol PTAL.

armiol

@dmytro-grankin please see my comments.

armiol · 2018-12-26T15:35:27Z

base/src/main/java/io/spine/code/js/MethodReference.java

+import static com.google.common.base.Preconditions.checkNotNull;
+
+/**
+ * A reference to a method.


To a method of what? Please update the description so that it's clear without analyzing the fields of this class.

armiol · 2018-12-26T15:37:06Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/GenerationTask.java

+     * @param fileSet
+     *         the Protobuf files to generate code for
+     */
+    public final void perform(FileSet fileSet) {


perform hardly reads with fileSet. It's either performAt or performFor.

armiol · 2018-12-26T15:37:30Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/GenerationTask.java

+     *
+     * @return {@code true} if there are files to process and {@code false} otherwise
+     */
+    private boolean shouldPerform(FileSet fileSet) {


Let's call it by what it really does, e.g. hasFiles.

armiol · 2018-12-26T15:38:51Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/output/snippet/Method.java

+/**
+ * The declaration of a method in Javascript code.
+ */
+public class Method implements Snippet {


It's weird to me that Method has no relation to MethodReference. They seem to describe the same thing twice.

armiol · 2018-12-26T15:42:36Z

tools/proto-js-plugin/src/test/resources/build.gradle

@@ -76,5 +76,6 @@ protobuf {
 build.dependsOn compileProtoToJs

 dependencies {
+    protobuf "io.spine:spine-base:${spineVersion}"


This seems wrong, as it references the "previously" published artifact, not the current one.

You can refer to base protos via filesystem if you need those to be taken into account during their generation. If you are not completely sure, let's discuss.

armiol · 2018-12-26T15:44:08Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/parse/ExportStandardParsersMap.java

@@ -58,7 +62,7 @@
 * <p>The parsers may be used to parse JSON via their {@code parse(value)} method.
 */
 @SuppressWarnings("OverlyCoupledClass") // Dependencies for the listed Protobuf types.
-public final class ProtoParsersGenerator extends JsCodeGenerator {
+public final class ExportStandardParsersMap implements Snippet {


I don't understand calling this snipped ending with Map. Will we need to rename the class if it stops generating a Map but generates a JS class instead?

dmytro-grankin · 2018-12-27T09:29:08Z

@armiol PTAL.

armiol

@dmytro-grankin LGTM with some minor changes to be made. And please create an issue to migrate from implementation dependency onto base artifact in JS tests.

armiol · 2018-12-27T14:57:18Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/output/IndentedLine.java

+     *         the value to adjust the depth by
+     * @return a new line with increased depth
+     */
+    IndentedLine adjustDepthBy(int depthChange) {


It's not called depth anymore. Let's fix the code and the documentation to reference an indentation level.

armiol · 2018-12-27T14:57:41Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/output/snippet/Comment.java

+import static com.google.common.base.Preconditions.checkNotNull;
+
+/**
+ * A single-lined comment.


single-line.

armiol · 2018-12-27T14:58:30Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/output/snippet/Comment.java

+
+    @Override
+    public String content() {
+        return "// " + text;


Let's not do that every time. This class is immutable, so we can compose the content right away.

armiol · 2018-12-27T14:59:40Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/output/snippet/Method.java

+
+    private final MethodReference reference;
+    private final List<String> arguments;
+    private final List<CodeLine> bodyLines;


Not sure why this isn't CodeLines.

I also think that this is just body.

armiol · 2018-12-27T15:02:15Z

tools/proto-js-plugin/src/main/java/io/spine/js/generate/output/snippet/Method.java

+public class Method implements Snippet {
+
+    private final MethodReference reference;
+    private final List<String> arguments;


"Arguments" are real values, "parameters" are declarations. I.e.

// `a` and `b` are the names of parameters. int sum(int a, int b) {...}; // `5` and `10` are arguments. int result = sum(5, 10);

I am not sure what you meant, so please adjust the code with the account of parameter/argument difference.

armiol · 2018-12-27T15:07:40Z

base/src/main/java/io/spine/code/generate/Generation.java

@@ -18,7 +18,7 @@
 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */

-package io.spine.code;
+package io.spine.code.generate;

 /**
 * Constants for code generation.


As long as we repackage.

Let's move ModelCompilerAnnotation to the top level, kill Generation (as it isn't what it claims to be) and rename ModelCompilerAnnotation to GeneratedBySpine. Ideally we need to relate this new class to the Generated annotation itself, e.g. return it from this class.

# Conflicts: # tools/model-compiler/src/main/java/io/spine/tools/gradle/compiler/ValidatingBuilderGenPlugin.java # tools/model-compiler/src/test/java/io/spine/tools/compiler/validation/VBuilderCodeTest.java

dmytro-grankin added 17 commits December 12, 2018 19:16

Rename MessageGenerator to FromJsonMethod

c6cad82

Rename JsFileWriter to JsFile

1e51abd

Extract a method

35736f9

Tell more about the actual actions in declaration names

730f439

Pass FileSet instead of a file with descriptors

2451e08

Extract FileSetEnhancement

e8d17a4

Move parsing-related generators into the same package

4345ff8

Move all code for generation of parsing to generate.parse

40df735

Create Given for fields

3396f9b

Move project-related code to GivenProject

544317a

Introduce TypeUrlsInFiles

ac35198

Rename JsonParsersWriter to ParsingOfObjects

185751b

Add tests for TypeUrlMethod

35071bc

Write tests for TypeUrlsInFile

24c4e00

Create TypeName using GenericDescriptor

8550fcb

Introduce JsType

5334afb

Reference main sources in tests since test sources are empty

258c187

dmytro-grankin self-assigned this Dec 17, 2018

dmytro-grankin added 2 commits December 17, 2018 15:44

Don't generate typeUrl() for prototypes

c8927ad

Merge branch 'master' into generate-type-url-for-js-messages

a6f5e96

dmytro-grankin added 2 commits December 17, 2018 18:31

Generate typeUrl method for definitions from options.proto

1ff8336

Rephrase Javadocs

5a19d24

dmytro-grankin requested a review from dmdashenkov December 18, 2018 08:36

dmdashenkov suggested changes Dec 18, 2018

View reviewed changes

dmytro-grankin added 4 commits December 18, 2018 12:26

Prevent instantiation of EnumType and MessageType

a33788f

outside of the package

Simplify a statement

932a1da

Rename TypesParsingExtension to TypeParsingExtension

7a72d81

Get rid of duplications in JsCode constructors

9db2dc3

Pass FileSet to GenerationTask.perform(...)

02826c2

SpineEventEngine deleted a comment Dec 25, 2018

dmytro-grankin added 2 commits December 25, 2018 13:52

Improve the API of MapExportSnippet

bd4cf5d

Improve description of packages, update Javadocs

343b012

SpineEventEngine deleted a comment Dec 26, 2018

dmytro-grankin added 2 commits December 26, 2018 10:48

Improve Javadocs

5029c30

Merge branch 'master' into generate-type-url-for-js-messages

24805b8

SpineEventEngine deleted a comment Dec 26, 2018

Move code generation related concepts under io.spine.code.generate

f9390cb

armiol requested changes Dec 26, 2018

View reviewed changes

dmytro-grankin added 4 commits December 26, 2018 17:55

Clarify naming and Javadocs for JS codegen

ccaec4c

Use MethodReference when composing a Method

0f10c00

Extract a method

3abe1ef

Provide enclosingRootDir property for test projects

8ef5816

SpineEventEngine deleted a comment Dec 26, 2018

dmytro-grankin added 2 commits December 26, 2018 20:46

Don't copy config and version.gradle for test projects

78f45ff

Update build.gradle for remaining test projects

4133e3d

armiol approved these changes Dec 27, 2018

View reviewed changes

dmytro-grankin added 6 commits December 27, 2018 17:59

Use "indent level" term instead of "depth"

0d46825

Compose the result of Comment.content() only once

a45b507

Improve naming related to Method

15600e9

Get rid of Generation and have just GeneratedBySpine

b5357e9

Merge branch 'master' into generate-type-url-for-js-messages

852ab64

# Conflicts: # tools/model-compiler/src/main/java/io/spine/tools/gradle/compiler/ValidatingBuilderGenPlugin.java # tools/model-compiler/src/test/java/io/spine/tools/compiler/validation/VBuilderCodeTest.java

Prepend a comment with slashes in the constructor

9eb7ce1

dmytro-grankin merged commit e876685 into master Dec 27, 2018

dmytro-grankin deleted the generate-type-url-for-js-messages branch December 27, 2018 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate a method providing `TypeUrl` for Protobuf types compiled to JS #255

Generate a method providing `TypeUrl` for Protobuf types compiled to JS #255

dmytro-grankin commented Dec 17, 2018 •

edited

Loading

codecov bot commented Dec 17, 2018 •

edited

Loading

dmytro-grankin commented Dec 18, 2018

dmdashenkov left a comment

dmdashenkov Dec 18, 2018

dmdashenkov Dec 18, 2018

dmdashenkov Dec 18, 2018

dmdashenkov Dec 18, 2018

dmytro-grankin Dec 18, 2018

dmdashenkov Dec 18, 2018

dmdashenkov Dec 18, 2018

dmdashenkov Dec 18, 2018

dmdashenkov Dec 18, 2018

dmytro-grankin commented Dec 26, 2018

armiol left a comment

armiol Dec 26, 2018

armiol Dec 26, 2018

armiol Dec 26, 2018

armiol Dec 26, 2018

armiol Dec 26, 2018

armiol Dec 26, 2018

dmytro-grankin commented Dec 27, 2018

armiol left a comment

armiol Dec 27, 2018

armiol Dec 27, 2018

armiol Dec 27, 2018

armiol Dec 27, 2018

armiol Dec 27, 2018

armiol Dec 27, 2018

	public final class TypesParsingExtension extends JsCodeGenerator {
	public final class TypeParsingExtension extends JsCodeGenerator {

Generate a method providing TypeUrl for Protobuf types compiled to JS #255

Generate a method providing TypeUrl for Protobuf types compiled to JS #255

Conversation

dmytro-grankin commented Dec 17, 2018 • edited Loading

codecov bot commented Dec 17, 2018 • edited Loading

Codecov Report

dmytro-grankin commented Dec 18, 2018

dmdashenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-grankin commented Dec 26, 2018

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-grankin commented Dec 27, 2018

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Generate a method providing `TypeUrl` for Protobuf types compiled to JS #255

Generate a method providing `TypeUrl` for Protobuf types compiled to JS #255

dmytro-grankin commented Dec 17, 2018 •

edited

Loading

codecov bot commented Dec 17, 2018 •

edited

Loading