[pigeon]: added generation of empty classes in dart

[feduke-nukem]

• Added generation of empty classes in dart
• Removed generating data class in Kotlin if class has no fields

closes 160501

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[tarrinneal]

You'll need to run the generate command dart run tools/generate.dart in the pigeon root folder before I can review this pr.

Thanks for putting this together!

[tarrinneal]

No real issues with the implementation here. Thanks for putting this together.

We will need an integration test for this though, you should be able to add it to the already existing 'event channel handles extended sealed classes' test in packages/pigeon/platform_tests/shared_test_plugin_code/lib/integration_tests.dart

The new version needs to be added to the pubspec and generator_tools.dart files as well. I assume those changes got lost during a merge from main.

We should land this one before your other pr to avoid having to update it again.

[tarrinneal]

This should be it's own version update 22.7.3

I think it should read Adds support for empty data classes
This shouldn't be added to dart if it isn't supported on all platforms also, so no [dart] needed

[feduke-nukem]

Done

Also added generating @Suppress("UNUSED_PARAMETER") for fromList() method of empty classes (Kotlin) as tests were failing due to linter

However I am facing issues at some CI steps:

Infra Failure: Step('Run package tests.reset XCode') (canceled) (retcode: None)
Task timed out.

UPD: It worked

[tarrinneal]

I'm realizing we don't have tests for sealed classes that aren't used in event channels (and therefor not tested on all platforms). I think adding it to the AllTypes family of classes is probably a good idea. I can do that part, if it's not something you would be interested in.

[feduke-nukem]

I can do that part, if it's not something you would be interested in.

Everything is fine, I am ready to do whatever is required.

But I should know exactly what is expected to be done?

Should I add new SealedAllTypes, SealedAllNullableTypes and SealedAllNullableTypesWithoutRecursion classes and create corresponding methods for testing like echoSomething or some new fields should be added to existing AllTypes classes to be able to reuse existing tests?

[tarrinneal]

I think just adding a Sealed class option to AllTypes and AllNullableTypes (and AllNullableTypesWithoutRecursion) should be sufficient. That's still a hefty change though as the methods that check for equality, and every test that creates one of those classes will need to be updated.

[tarrinneal]

Add a different sub class as the default version of each class to get more coverage (including an empty class of course)

[feduke-nukem]

I think just adding a Sealed class option to AllTypes and AllNullableTypes (and AllNullableTypesWithoutRecursion) should be sufficient. That's still a hefty change though as the methods that check for equality, and every test that creates one of those classes will need to be updated.

So I need to add a single parent sealed class and make AllTypes, AllNullableTypes, AllNullableTypesWithoutRecursion it's subclasses plus additional empty subclass AllTypesEmpty.

Did I get it right?

Or each of the classes AllTypes, AllNullableTypes and AllNullableTypesWithoutRecursion should itself become sealed with one subclass actually filled with assosiated "types" and second empty without any fields?

Sorry for questioning a lot 🙂

[tarrinneal]

I meant add a field in the AllTypes class that is the sealed class.

And 100% no worries at all about asking questions, I'm just happy you're working on pigeon :)

I'm very happy to help/assist/answer in any way you need.

[tarrinneal]

AllTypes(anInt, aString,..., aSealedClass)

[feduke-nukem]

AllTypes(anInt, aString,..., aSealedClass)

Got it.

Last clarification before implementation.

Should aSealed class have subclasses containing basic types/structures like in event channel tests like: IntClass extends SealedClass, StringClass extends SealedClass, EmptyClass extends SealedClass etc? Or should it be like AllTypesSealedClass filled with copy/pasted properties from AllTypes (same for others AllTypes classes) to test proper serialization/deserialization?