MarimerLLC / cslaforum

Discussion forum for CSLA .NET
https://cslanet.com
Other
31 stars 6 forks source link

INPUT NEEDED: RegisterProperty overloads #746

Closed rockfordlhotka closed 5 years ago

rockfordlhotka commented 5 years ago

We're planning to change the way RegisterProperty works in version 5 to make use of the nameof operator.

https://github.com/MarimerLLC/csla/issues/1101

@JasonBock is planning to create an analyzer that should make this migration pretty painless.

However, I've run into a bit of complexity because I created so many (too many?) overloads of RegisterProperty in the past and I can't disambiguate them in all cases. Actually this has been an issue in the past, but with this change it becomes much worse.

Today:

  1. RegisterProperty(PropertyInfo x)
  2. RegisterProperty(Type t, PropertyInfo x)
  3. RegisterProperty(Lamba x)
  4. RegisterProperty(Lamba x, RelationshipTypes r)
  5. RegisterProperty(Lamba x, defaultValue d)
  6. RegisterProperty(Lamba x, FriendlyName n)
  7. RegisterProperty(Lamba x, FriendlyName n, RelationshipTypes r)
  8. RegisterProperty(Lamba x, FriendlyName n, defaultValue d)
  9. RegisterProperty(Lamba x, FriendlyName n, defaultValue d, RelationshipTypes r)

And today if a property is of type string then there are several ambiguities. The same if the property is of type object.

I suspect few of these are used. I think I was crazy for implementing all those overloads - wow!

1 and 2 are used for inheritance scenarios, so necessary.

3 is the most common. I suspect 4 is pretty common.

5 and 6 are a mess - probably often ambiguous and so rarely used (I hope???).

7 could be useful maybe? I don't know. 8 seems reasonably useful. However, 7 and 8 can be ambiguous for a property of type object.

9 has no ambiguity and so is probably fine.

Question: what if I just kept 1, 2, 3, 4, 6, and 9?

Or worst case, I'll only implement nameof overloads for 3, 4, 6, 8, and 9. Which means Jason's analyzer can correct for those, but not 5 or 7.

Proposed future:

  1. RegisterProperty(PropertyInfo x)
  2. RegisterProperty(Type t, PropertyInfo x)
  3. RegisterProperty(string propertyName)
  4. RegisterProperty(string propertyName, RelationshipTypes r)
  5. RegisterProperty(string propertyName, FriendlyName n)
  6. RegisterProperty(string propertyName, FriendlyName n, defaultValue d)
  7. RegisterProperty(string propertyName, FriendlyName n, defaultValue d, RelationshipTypes r)
ghost commented 5 years ago

I’d have to get back you on which versions are used in my projects but I would think splitting the overloads would be bad and maybe confusing? Just doesn’t seem like a good idea to me.

rockfordlhotka commented 5 years ago

The idea is to help people migrate off the lambda expression versions via the analyzer and then remove them entirely.

Right now what I'm thinking is to create as many overloads as possible where ambiguity doesn't exist, leaving the existing lambda ones, and marking the ones that can't carry forward as Obsolete.

The two obsolete ones are

  1. RegisterProperty(Lamba x, defaultValue d)
  2. RegisterProperty(Lamba x, FriendlyName n, RelationshipTypes r)

They'll have no equivalent in the new model, because they can't be independently resolved from the others, so I'll mark these as Obsolete to make it clear they are no longer valid.

The other lambda-based ones will remain entirely valid, but the analyzer will flag them and provide a correction to change to the new nameof technique.

GillesBer commented 5 years ago

Hi @rockfordlhotka , all, Unfortunately, I noticed that we are using some of the (soon) obsolete RegisterProperty, such as: RegisterProperty((c) => c.Title, "Business card title"); //expecting it to be the 'friendlyname', and not the 'default value'.

If you were to refactor, what would be the recommend way? Woud using [Display] or [DisplayName] more recommended than using the overload? I'm resurecting an old related thread at https://cslanet.com/old-forum/11874.html

Also, I believe that the same recommendation would apply to the PropertyInfo constructors. We still have some legacy:

public static readonly PropertyInfo CreationByProperty = RegisterProperty(new PropertyInfo("CreationBy", "Created by", "")); public string CreationBy { get { return GetProperty(CreationByProperty); } set{ SetProperty(CreationByProperty, value); } }

rockfordlhotka commented 5 years ago

Thank you for the feedback @GillesBer

Right now it looks like the following overloads will exist:

So I think you are OK - there appear to be direct maps from your existing code to the new nameof() overloads.