Handle `compare_by` option #154

yevhenii-nadtochii · 2024-08-27T15:34:53Z

This PR adds support for Comparable messages (#152).

Now, we have mc-java-comparable plugin, which discovers messages with compare_by option and generates code to make them comparable.

The following render actions are applied:

ImplementComparable – adds Comparable<...> interface to the message.
AddComparator – adds a private static field containing the Comparator<...> itself.
AddCompareTo – adds an overridden method accordingly to the interface contract.

External messages

An external message is a message, which is not being generated by the current codegen session. Usually, they come from the dependencies. For example, google.protobuf.Timestamp. When a user wants to use a field in comparison, the type of which is external, a comparator should be provided within the ComparatorRegistry. By default, the registry provides comparators for Timestamp and Duration only.

If the external message has compare_by option (our messages, generated by mc-java as well), then the presence of comparator is not required. Though, the plugin will throw if there is the option and comparator in the registry simultaneously.

Concerns

AddComparator: this is the smartest action. It performs analysis of Proto messages, indeed. I'm not sure if it should be done in an entity of DirectMessageAction type.
ComparableActionsRenderer: can we store actions in one place? Our model doesn't specify a distinct set of actions for each message, and probably wouldn't. We carry the same collection of action classes in each discovered message. One could think that each message may have its own set of actions.

Other changes

implementsInterface assertion is stricter now, requiring the asserted interface to be located before an opening brace of the class body.

…sages

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/AddCompareToMethod.kt

yevhenii-nadtochii · 2024-10-15T09:20:10Z

@armiol @alexander-yevsyukov PTAL

Also, I suggest overriding RenderAction.context and RenderAction.typeSystem, so that they are non-nullable for actions. Mark them as open in Member, and override in RenderAction. The action calls registerWith(context) in its init block.

armiol

@yevhenii-nadtochii please see my comments so far. I will continue the review in IDE, as it's difficult to follow via GitHub.

armiol · 2024-10-15T09:51:55Z

...ava-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/ComparatorBuilder.kt

+ * ```
+ *
+ * Please note the line breaks were added manually for reader's convenience.
+ * The builder doesn't add them. It will be a single line of text.


After the code is automatically formatted by PSI, will it still be a single line of text?

I see in integration tests output that the comparator remains unformatted.

This comment relates to this particular builder so that no one expected build(): String to return a string as from the KDoc snippet.

armiol · 2024-10-15T09:53:12Z

...ava-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/ComparatorBuilder.kt

+ * The builder doesn't add them. It will be a single line of text.
+ *
+ * @param cls The message class to be used as comparator's generic parameter.
+ * @param descending If true, the default order is reversed.


Which order is a "default" one?

armiol · 2024-10-15T09:53:39Z

...ava-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/ComparatorBuilder.kt

+ * The builder doesn't add them. It will be a single line of text.
+ *
+ * @param cls The message class to be used as comparator's generic parameter.
+ * @param descending If true, the default order is reversed.


I am not sure, but if you explain "descending" as "in a reversed order", maybe the parameter should be called something like reversed?

And honestly, "descending" is more about sorting to me.

mc-java-base/src/main/kotlin/io/spine/tools/mc/java/base/FieldPathExts.kt

alexander-yevsyukov

Please see my comments.

alexander-yevsyukov · 2024-10-15T10:20:48Z

mc-java-base/src/main/kotlin/io/spine/tools/mc/java/base/FieldPathExts.kt

+    val currentField = message.field(fieldPath.root)
+    val remainingFields = fieldPath.fieldNameList.drop(1)
+    val remainingPath = fieldPath { fieldName.addAll(remainingFields) }
+    val nextMessage = findMessage(currentField.type.message)!!.first


What if the referenced field is not of message type? I believe we need to tell about such an error.

alexander-yevsyukov · 2024-10-15T10:21:26Z

mc-java-base/src/main/kotlin/io/spine/tools/mc/java/base/MessageTypeExts.kt

+ * The function returns a non-`null` result if a class denoted by this [ClassName]
+ * is present on the classpath.
+ */
+public fun MessageType.javaClass(accordingTo: ProtoFileHeader): Class<*>? {


This extension should go to ProtoData too.

alexander-yevsyukov · 2024-10-15T10:24:05Z

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

+            }
+
+            else -> error(
+                "The field `$path` has an unrecognized Proto field type: `$type`. " +


We need to standardize on capitalization of the word "proto". I think in the short form it should go in lowercase, while "Protobuf" is capitalized. This is the way Google use it in their sources, from what I remember.

Also, I'm not sure about "unrecognized". We do recognize the type because it's in the source code, descriptors, and is (at least) proto-compilable. Otherwise, we wouldn't have got this far.

But this type isn't probably "comparable" because it's a list, or a map.

This unrecognized is a stub error thrown when ast.Type has an unknown kind. It is not about a non-comparable type.

kind is represented by oneof, which has nothing to do with Kotlin's when (as for example, with sealed classes):

message Type { oneof kind { option (is_required) = true; TypeName message = 1; TypeName enumeration = 2; PrimitiveType primitive = 3; } }

So, we have to provide else branch in case some new kind appears.

OK. I get it. The problem it is buried inside extensions, and is not obvious from the branching here. Hence, my comment.

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

alexander-yevsyukov · 2024-10-15T10:35:04Z

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

+
+            else -> error(
+                "The field `$path` has an unrecognized Proto field type: `$type`. " +
+                        "The supported Proto fields: primitives, enums and messages."


Suppose you're the person who gets this error message in console. What it would take to figure out the root cause of the error. Let's sum what we see in the diag message:

"Unrecognized type".

It's not "supported".

Supported types are primitives, enums, and messages. Please notice Oxford comma in the enumeration.

Stacktrace having "comparable".

In the end the combination above would give the reader idea that the field isn't probably comparable.
But can we make this path to the conclusion a bit shorter?

Discussed the improved wording for the error message:

The field `$path` declared in the message type `$messageType` is not of the supported type (`$fieldType`) for the comparison. Supported field types are: primitives, enums, and messages.

alexander-yevsyukov · 2024-10-15T10:36:25Z

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

+     * Adds the comparison [field] to this [ComparatorBuilder].
+     *
+     * The requirements to the comparison fields are described in docs to [CompareByOption]
+     * option in detail.


The reference to documentation details should probably be a part of the error diagnostics I mentioned above.

alexander-yevsyukov · 2024-10-15T10:37:28Z

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

+        when {
+            hasCompareByOption -> {
+                check(fromRegistry == null) {
+                    "The type of `${path.joined}` field must either have `compare_by` options OR " +


This message is good!

Please also consider my comment about braces around the option name here.

alexander-yevsyukov · 2024-10-15T10:39:04Z

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

+
+            fromRegistry != null -> {
+                val className = "${clazz.canonicalName}.class"
+                val comparator = "io.spine.compare.ComparatorRegistry.get($className)"


Can we use the reference extension to the ComparatorRegistry class here? This way finding usages would pick up this dependency.

...ava-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/ComparatorBuilder.kt

...ava-comparable/src/test/java/io/spine/tools/mc/java/comparable/tests/env/LocalDateTimes.java

yevhenii-nadtochii · 2024-10-16T11:09:59Z

@armiol @alexander-yevsyukov PTAL

I will try bringing typed expressions to ComparatorBuilder with a separate PR. TypeSystem.resolve(path) also would go there when it is published.

alexander-yevsyukov

LGTM with a couple of comments regarding diag. messages to consider before merging.

alexander-yevsyukov · 2024-10-16T11:16:43Z

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

+
+            is MessageComparisonField -> {
+                check(field.type.hasCompareByOption) {
+                    "The type of `${path.joined}` field should have `compare_by` option itself " +


We usually put the option in round braces when mentioning in the docs or diagnostic messages. I don't know who started this, and we probably don't have it standardized in our guidelines. But (compare_by) looks more visible and is clearly different to a field.

Also, the definite article should be probably put before the name.

@armiol, for your attention.

alexander-yevsyukov · 2024-10-16T11:17:20Z

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

+        when {
+            hasCompareByOption -> {
+                check(fromRegistry == null) {
+                    "The type of `${path.joined}` field must either have `compare_by` options OR " +


Please also consider my comment about braces around the option name here.

alexander-yevsyukov · 2024-10-16T11:17:57Z

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/action/AddComparator.kt

+    private fun unsupportedFieldType(fieldPath: FieldPath, fieldType: Any?): Nothing = error(
+        "The field `$fieldPath` declared in the message `$type` is not of the supported type " +
+                " (`$fieldType`) for the comparison. Supported field types are: " +
+                "primitives, enums, and comparable messages. Checks out docs to `compare_by` " +


Please consider braces around the option name in this message.

armiol

@yevhenii-nadtochii LGTM.

However, please see my comment. Maybe, that's something we need to discuss.

armiol · 2024-10-16T11:32:13Z

mc-java-comparable-tests/src/test/kotlin/io/spine/tools/mc/java/comparable/AddComparatorSpec.kt

+
+    @Nested
+    inner class
+    `not generate comparator` {


What do we think of oneof?

It's now in a gray zone. I would either have tests explicitly covering our support or our refusal to support the comparison by oneof fields.

I've added the negative tests to cover oneof. Its support was not declared.

yevhenii-nadtochii added 16 commits August 23, 2024 14:02

Add proto messages with compare_by option

440c1c8

Implement a test that checks the message implements Comparable

079cc5a

Implement ComparableMessageDiscovery

408bf36

Register a new module in Gradle

35fab00

Implement ComparableMessageView

2ba1be1

Implement ComparableActionRenderer

25383bc

Implement ImplementComparable

741ce71

Append a primitive implementation of compareTo() for comparable mes…

586c59e

…sages

Fix long lines

c99629f

Fix the message package

4e3b4b8

Bump the version → 2.0.0-SNAPSHOT.218

fe93de9

Put missing Spine annotations

edc39a6

Store file along with type name

d96a7d1

Propagate .proto changes to entities

79c37e7

Roll back to TypeDiscovered event

dba9a22

Implement a renderer from scratch

77a8cd1

yevhenii-nadtochii self-assigned this Aug 27, 2024

yevhenii-nadtochii linked an issue Aug 27, 2024 that may be closed by this pull request

Implement support for Comparable messages #152

Closed

alexander-yevsyukov reviewed Aug 27, 2024

View reviewed changes

mc-java-comparable/src/main/kotlin/io/spine/tools/mc/java/comparable/AddCompareToMethod.kt Outdated Show resolved Hide resolved

yevhenii-nadtochii added 11 commits August 28, 2024 12:20

Bump gradle-doctor to 0.10.0

26c58cd

Implement ComparableSettings

50294d3

Make comparable settings configurable from codegen

91524dd

Propagate setting to the plugin model

a132596

Write comparables settings to a file

5c2d3e1

Use TypeRenderer for rendering

ef7f672

Test ComparableSettings

6c0c6dc

Test generation of implementing Comparable

6549ba7

Make the used regex expression stricter

d0514b8

Assert the generic parameter as well

8e7427e

Remove an empty line

3ce3e0a

yevhenii-nadtochii added 5 commits October 14, 2024 16:49

Substitute FieldLookup with an extension function

3855ba6

Actualize docs

9fc8b71

Enhance docs to resolve()

457a34b

Proofread docs

5b732f5

Make hasCompareByOption a private extension

5cf0fd8

yevhenii-nadtochii requested review from alexander-yevsyukov and armiol October 15, 2024 09:20

armiol requested changes Oct 15, 2024

View reviewed changes

Rename descending to reversed

3ae4f9c

alexander-yevsyukov requested changes Oct 15, 2024

View reviewed changes

yevhenii-nadtochii added 9 commits October 15, 2024 12:53

Update docs to ComparatorBuilder.reversed

0f19bad

Add more checks to TypeSystem.resolve()

44321f5

Use either proto or Protobuf

546f58e

Use reference extension

8d19afb

Mention that the used LocalDateTime is not Spine-provided

1118d91

Bump ProtoData -> 0.61.7

ff787c2

Remove unnecessary non-null assertions

b616520

Remove local implementation of MessageType.javaClass()

1d57415

Extract unsupportedFieldType() method with enhanced diagnostic message

c6184c6

yevhenii-nadtochii requested review from alexander-yevsyukov and armiol October 16, 2024 11:10

alexander-yevsyukov approved these changes Oct 16, 2024

View reviewed changes

armiol approved these changes Oct 16, 2024

View reviewed changes

yevhenii-nadtochii added 3 commits October 16, 2024 14:08

Add negative test cases for oneof fields

81e9f7a

Put braces around the option name

ef9f94f

Update reports

c2afd1f

yevhenii-nadtochii merged commit 867cb60 into master Oct 16, 2024
7 checks passed

yevhenii-nadtochii deleted the handle-comparable-option branch October 16, 2024 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `compare_by` option #154

Handle `compare_by` option #154

yevhenii-nadtochii commented Aug 27, 2024 •

edited

Loading

yevhenii-nadtochii commented Oct 15, 2024

armiol left a comment

armiol Oct 15, 2024

yevhenii-nadtochii Oct 15, 2024

armiol Oct 15, 2024

armiol Oct 15, 2024

alexander-yevsyukov left a comment

alexander-yevsyukov Oct 15, 2024

alexander-yevsyukov Oct 15, 2024

alexander-yevsyukov Oct 15, 2024

alexander-yevsyukov Oct 15, 2024

yevhenii-nadtochii Oct 15, 2024

alexander-yevsyukov Oct 15, 2024

alexander-yevsyukov Oct 15, 2024

alexander-yevsyukov Oct 15, 2024 •

edited

Loading

alexander-yevsyukov Oct 15, 2024

alexander-yevsyukov Oct 15, 2024

alexander-yevsyukov Oct 16, 2024

alexander-yevsyukov Oct 15, 2024

yevhenii-nadtochii commented Oct 16, 2024

alexander-yevsyukov left a comment

alexander-yevsyukov Oct 16, 2024

alexander-yevsyukov Oct 16, 2024

alexander-yevsyukov Oct 16, 2024

alexander-yevsyukov Oct 16, 2024

armiol left a comment

armiol Oct 16, 2024

yevhenii-nadtochii Oct 16, 2024

Handle compare_by option #154

Handle compare_by option #154

Conversation

yevhenii-nadtochii commented Aug 27, 2024 • edited Loading

External messages

Concerns

Other changes

yevhenii-nadtochii commented Oct 15, 2024

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

alexander-yevsyukov 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

alexander-yevsyukov Oct 15, 2024 • edited Loading

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

yevhenii-nadtochii commented Oct 16, 2024

alexander-yevsyukov 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

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

Handle `compare_by` option #154

Handle `compare_by` option #154

yevhenii-nadtochii commented Aug 27, 2024 •

edited

Loading

alexander-yevsyukov Oct 15, 2024 •

edited

Loading