mono
NameGenerator now honors the RegisterAttribute
Resolves mono/Embeddinator-4000#481 by allowing users to override the generated objc class name by decorating their managed class with the RegisterAttribute.
Hello! I'm the build bot for the Mono project.
I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Additional trigger words are available here.
Contributors can ignore this message.
Hello! Glad to see you contributing!! I am not entirely sold on reusing 'RegisterAttribute' for this because it could potentially interfere with the Xamarin registrar/runtime. Maybe we'll need embeddinator specific attributes.
Thanks, old friend! FWIW, I chose to re-use it since the official Xamarin docs state that RegisterAttribute is now used solely to customize the objc class name emitted by the XI generator. Adding a second attribute with the same responsibility would be a DRY violation. If I have a XI assembly using them I'd want it to Just Work. Having two attributes decorating the same class with the same values would be mildly awkward.
FWIW we also rely on [Register] for this purpose in the Java backend, a behaviour we inherit from Xamarin.Android Java interop implementation.
Awesome, agree on the intention of reusing it, if @spouliot and @rolfbjarne are ok with this I am too!
I'm not sure if abusing the Register attribute is a good idea.
First, it means that libraries that want to customize their class names must reference Xamarin.Mac or Xamarin.iOS which is a heavy dependency if you just want to rename a method for use with ObjC.
Second, it only works on classes but I'd also like to be able to rename methods and properties.
And third: I think a dedicated (thin) E4k support library would make sense for other features too that wouldn't be available in Xamarin.Mac or iOS already. Like a new attribute to tell E4k to not generate bindings for specific classes, methods or properties.
I'm fine with using [RegisterAttribute]. However I think you should be more defensive (it might not have an argument ([Register]), in which case your code will throw an exception. I'd do something like this, except that I'd issue a warning instead of an error if some assumption (maybe someday there will be 3 arguments?) breaks:
https://github.com/xamarin/xamarin-macios/blob/a001cace6f1e77646cd63e0119ab344f3935851b/tools/common/StaticRegistrar.cs#L1109-L1143
@lemonmojo note that this code only does a string comparison for the attribute name; this means that you don't have to reference Xamarin.iOS/Xamarin.Mac, you can define your own RegisterAttribute class.
@rolfbjarne I like it. I'll update the PR. With respect to the other params, like IsWrapper, should the switch ignore them, log them at verbose log level, or some third option I'm not able to think of?
@zgramana Just ignore the IsWrapperfield (no logging needed), it doesn't matter for the embeddinator (and I think you can ignore the SkipRegistration field as well, at least for now, but this one you might want to log, since users might want to try to use it for the obvious purpose). You might want to log other (unknown) fields as well.
Actually, hold off for another commit forthcoming. Apologies for the false start.
Okay, now it's ready for review. In addition to the possible failure modes @rolfbjarne indicated, I also made sure to handle the case of conflicting name values, e.g. [Register("myFoo", Name = "foo")] as well as validating the layout of non-Xamarin.[iOS|Mac] provided Register attributes for users like @lemonmojo who might write their own.
@zgramana @rolfbjarne Thx for clarifying that there's no external dependency required for this. In this case the PR sounds good to me.
I'd still like to be able to also rename specific properties/methods/etc. and exclude certain classes/properties/etc. from being generated but I guess that's fuel for another Issue/PR.
@zgramana Just noticed a small typo in the updated documentation: Users already familiar with the RegisterAttribute might expect SkipRegistration to function ---> is <--- it does in Xamarin.iOS or Xamarin.Android. However, this is unsupported and the value of SkipRegistration will be ignored.
@rolfbjarne I've pushed up the copy edits you and @lemonmojo found.
Unfortunately I'm going to have to leave the tests to someone else as I have to attend to business commitments. Hope this is PR is still helpful.
