packages icon indicating copy to clipboard operation
packages copied to clipboard

[pigeon] Fixes double prefixes added to enum names for Objc HostApis and FlutterApis

Open bparrishMines opened this issue 1 year ago • 7 comments
trafficstars

In some areas of the Objc generator, the the name created from _objcTypeForDartType is being passed to _enumName and both of these methods add a prefix to enums. The locations are when they are used in HostApi or FlutterApi methods, but the name of the generated enum would be correct. e.g. FLTEnumName vs FLTFLTEnumName.

This fixes the locations where this is happening by passing the AST name to _enumName instead.

Pre-launch Checklist

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

bparrishMines avatar Mar 05 '24 00:03 bparrishMines

What scenario is missing from our integration tests that would have triggered this?

tarrinneal avatar Mar 06 '24 00:03 tarrinneal

@tarrinneal It looks like the scenario is a combination of:

  1. Using a primitive enum as a method parameter or return value
  2. Generates Objective-c code with a prefix

It looks like CoreTests.gen.h doesn't use a prefix. Although scenario 1 does occur for it.

Would it be a good idea to update core tests with a prefix?

bparrishMines avatar Mar 06 '24 00:03 bparrishMines

Would it be a good idea to update core tests with a prefix?

yup

tarrinneal avatar Mar 06 '24 00:03 tarrinneal

@tarrinneal Adding the prefix to core tests also found a location where the prefix wasn't added to the enum. I think a better solution might be to actually get rid of the _enumName method altogether and just use _ObjcType instead. It seems redundant to _ObjcType and it seems to be error prone to have both. I will try and explore this option. What are your thoughts?

bparrishMines avatar Mar 06 '24 01:03 bparrishMines

Actually it looks like the problem is bigger than enums. Objective-c prefixes may need a bigger fix.

bparrishMines avatar Mar 06 '24 01:03 bparrishMines

I can take this over if you'd like.

tarrinneal avatar Mar 06 '24 02:03 tarrinneal

@stuartmorgan this is ready for final review

tarrinneal avatar Mar 12 '24 14:03 tarrinneal