NameGenerator now honors the RegisterAttribute #482

zgramana · 2017-08-17T00:13:32Z

Resolves #481 by allowing users to override
the generated objc class name by decorating their managed class
with the RegisterAttribute.

Resolves mono#481 by allowing users to override the generated objc class name by decorating their managed class with the RegisterAttribute.

monojenkins · 2017-08-17T00:17:42Z

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.

tritao · 2017-08-17T00:20:53Z

whitelist

dalexsoto · 2017-08-17T01:28:21Z

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.

zgramana · 2017-08-17T02:48:35Z

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.

tritao · 2017-08-17T02:51:45Z

FWIW we also rely on [Register] for this purpose in the Java backend, a behaviour we inherit from Xamarin.Android Java interop implementation.

dalexsoto · 2017-08-17T03:17:20Z

Awesome, agree on the intention of reusing it, if @spouliot and @rolfbjarne are ok with this I am too!

lemonmojo · 2017-08-17T07:42:49Z

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.

rolfbjarne · 2017-08-17T07:54:14Z

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.

zgramana · 2017-08-17T15:40:04Z

@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?

rolfbjarne · 2017-08-17T15:45:26Z

@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.

In response to code review feedback, handled RegisterAttribute in the spirit in which StaticRegistrar.cs does in xamarin-macios. Also includes documentation of newly added error codes.

zgramana · 2017-08-17T19:15:42Z

I just pushed a refactor based on @rolfbjarne's feedback for review.

zgramana · 2017-08-17T19:17:53Z

Actually, hold off for another commit forthcoming. Apologies for the false start.

The previous refactor ommitted the null check after looking for RegisterAttribute. This commit adds it back in.

zgramana · 2017-08-17T19:25:00Z

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.

lemonmojo · 2017-08-18T07:55:49Z

@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.

lemonmojo · 2017-08-18T08:23:12Z

@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

A few minor comments, and additionally it would be nice to get tests for this as well.

rolfbjarne · 2017-08-18T09:13:42Z

docs/errors.md

+
+If an argument was not also passed for named parameter "Name" then the class will be named according to the default convention.
+
+<h3><a name="EM1061"/>Invalid `RegisterAttribute` found on class `T`. Expected a constructor with either 1 or 2 parameters but found {argsCount} instead..</h3>


Typo: two periods at the end of the sentence.

rolfbjarne · 2017-08-18T09:14:10Z

docs/errors.md

@@ -217,6 +217,53 @@ Selectors on the [NSObjectProtocol](https://developer.apple.com/reference/object

 Note: The list of reserved selectors will evolve with new versions of the tool.

+<h3><a name="EM1060"/>Invalid `RegisterAttribute` found on class `T`. Expected the first constructor parameter to be of type `string` but found type `U` instead.</h3>
+
+This is a **warning** that the class `T` was decorated with a `RegisterAttribute`, indicating the author wishes to rename the name of generated objective-c class.


Minor: use proper casing Objective-C instead of all lowercase (there are also more cases below).

rolfbjarne · 2017-08-18T09:16:14Z

docs/errors.md

+
+This is a **warning** that the class `T` was decorated with a `RegisterAttribute`, indicating the author wishes to rename the name of generated objective-c class.
+
+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.


I'm not sure SkipRegistration is applicable to Xamarin.Android (I don't know anything about XA). Did you mean Xamarin.Mac instead of Xamarin.Android?

Yes, I did, thanks for catching that. My fingers seem to have their own L4 cache…

zgramana · 2017-08-18T15:03:42Z

@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.

dnfclas · 2018-02-06T18:34:32Z

All CLA requirements met.

NameGenerator now honors the RegisterAttribute

86e4d8c

Resolves mono#481 by allowing users to override the generated objc class name by decorating their managed class with the RegisterAttribute.

tritao requested a review from spouliot August 17, 2017 00:20

dalexsoto requested a review from rolfbjarne August 17, 2017 01:29

More robust handling of RegisterAttribute.

e90825d

In response to code review feedback, handled RegisterAttribute in the spirit in which StaticRegistrar.cs does in xamarin-macios. Also includes documentation of newly added error codes.

Added back missing null check in GetObjCName.

31f9613

The previous refactor ommitted the null check after looking for RegisterAttribute. This commit adds it back in.

lemonmojo mentioned this pull request Aug 18, 2017

[objc] Enable method/property name customization via ExportAttribute #483

Closed

rolfbjarne suggested changes Aug 18, 2017

View reviewed changes

Errors.md: edits incorporating review feedback

cd93f96

zgramana added 3 commits October 2, 2017 17:06

Merge branch 'master' of https://github.com/mono/Embeddinator-4000

4f09d71

Merge branch 'master' of https://github.com/mono/Embeddinator-4000

7d0135c

Merge branch 'master' of https://github.com/mono/Embeddinator-4000

4f4eb77

chamons mentioned this pull request Mar 19, 2018

[objc] Enable class name customization via RegisterAttribute #481

Open

Base automatically changed from master to main March 9, 2021 10:25

akoeplinger requested review from chamons, dalexsoto and jonathanpeppers as code owners March 9, 2021 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NameGenerator now honors the RegisterAttribute #482

NameGenerator now honors the RegisterAttribute #482

zgramana commented Aug 17, 2017

monojenkins commented Aug 17, 2017

tritao commented Aug 17, 2017

dalexsoto commented Aug 17, 2017

zgramana commented Aug 17, 2017

tritao commented Aug 17, 2017

dalexsoto commented Aug 17, 2017

lemonmojo commented Aug 17, 2017

rolfbjarne commented Aug 17, 2017 •

edited

Loading

zgramana commented Aug 17, 2017

rolfbjarne commented Aug 17, 2017

zgramana commented Aug 17, 2017 •

edited

Loading

zgramana commented Aug 17, 2017

zgramana commented Aug 17, 2017 •

edited

Loading

lemonmojo commented Aug 18, 2017

lemonmojo commented Aug 18, 2017

rolfbjarne left a comment

rolfbjarne Aug 18, 2017

rolfbjarne Aug 18, 2017

rolfbjarne Aug 18, 2017

zgramana Aug 18, 2017

zgramana commented Aug 18, 2017

dnfclas commented Feb 6, 2018 •

edited

Loading


		If an argument was not also passed for named parameter "Name" then the class will be named according to the default convention.

		<h3><a name="EM1061"/>Invalid `RegisterAttribute` found on class `T`. Expected a constructor with either 1 or 2 parameters but found {argsCount} instead..</h3>


		This is a warning that the class `T` was decorated with a `RegisterAttribute`, indicating the author wishes to rename the name of generated objective-c class.

		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.

NameGenerator now honors the RegisterAttribute #482

Are you sure you want to change the base?

NameGenerator now honors the RegisterAttribute #482

Conversation

zgramana commented Aug 17, 2017

monojenkins commented Aug 17, 2017

tritao commented Aug 17, 2017

dalexsoto commented Aug 17, 2017

zgramana commented Aug 17, 2017

tritao commented Aug 17, 2017

dalexsoto commented Aug 17, 2017

lemonmojo commented Aug 17, 2017

rolfbjarne commented Aug 17, 2017 • edited Loading

zgramana commented Aug 17, 2017

rolfbjarne commented Aug 17, 2017

zgramana commented Aug 17, 2017 • edited Loading

zgramana commented Aug 17, 2017

zgramana commented Aug 17, 2017 • edited Loading

lemonmojo commented Aug 18, 2017

lemonmojo commented Aug 18, 2017

rolfbjarne left a comment

Choose a reason for hiding this comment

rolfbjarne Aug 18, 2017

Choose a reason for hiding this comment

rolfbjarne Aug 18, 2017

Choose a reason for hiding this comment

rolfbjarne Aug 18, 2017

Choose a reason for hiding this comment

zgramana Aug 18, 2017

Choose a reason for hiding this comment

zgramana commented Aug 18, 2017

dnfclas commented Feb 6, 2018 • edited Loading

rolfbjarne commented Aug 17, 2017 •

edited

Loading

zgramana commented Aug 17, 2017 •

edited

Loading

zgramana commented Aug 17, 2017 •

edited

Loading

dnfclas commented Feb 6, 2018 •

edited

Loading