Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement support for inherited properties #50

Merged
merged 15 commits into from
May 2, 2024
Merged

implement support for inherited properties #50

merged 15 commits into from
May 2, 2024

Conversation

Ashymonth
Copy link

Add the ability to generate inherited properties
In order not to break compatibility with previous versions and to allow the user to choose for himself how to act with the properties that are in the base class, the InheritedColumnsOrdering attribute was added. You can use it to set 3 types of behavior for inherited properties.

  • IgnoreInheritedProperties - properties from base class will be igonred. This is default behaviour.
  • StartFromInheritedProperties - Property generation will start with the properties of the base class.
  • StartFromClassProperties - Property generation will start from the current class

@Ashymonth
Copy link
Author

Ashymonth commented Apr 25, 2024

i also added test for new staff. I tried to follow your code style. If you want to change smth i can do it.
I also changed classType.GetMembers() to classType.GetMembers().OfType<IPropertySymbol>();, but if you say to revert I'll do it

@sveinungf
Copy link
Owner

Thanks for the PR.

  1. It looks like the code handles properties from the direct base class. But what if there are two or more levels of inheritance?
  2. I'm a bit unsure about the attribute name InheritedColumnOrdering. I think in some cases you would want to include the properties from the base class, but not necessarily set the column ordering. For example if all properties have the ColumnOrderAttribute set, then having to also decide the ordering for this attribute can be a bit confusing. Maybe the name could basically be something that means "take the base class into account". The column ordering could be an optional parameter for the attribute.
  3. Ideally the name should make it clear that it sets the default ordering. Any use of the ColumnOrderAttribute can be used to override the default order.

Perhaps a way to solve these issues could be to have the attribute named InheritColumns, with optional parameter DefaultOrder which could be either InheritedColumnOrder.InheritedColumnsFirst or InheritedColumnOrder.InheritedColumnsLast. If there are more levels of inheritance, you would also have to set the attribute on the base class, and optionally the parameter as well.

An example of how this could work:

public class Animal
{
    public DateTime DateOfBirth { get; set; }
}

[InheritColumns(DefaultOrder = InheritedColumnOrder.InheritedColumnsLast)]
public class Mammal : Animal
{
    public bool CanWalk { get; set; }
}

[InheritColumns] // Defaults to InheritedColumnsFirst
public class Dog : Mammal
{
    public string Breed { get; set; }
}

The order of the columns here should be: CanWalk, DateOfBirth, Breed.

@Ashymonth
Copy link
Author

Ashymonth commented Apr 26, 2024

Thanks for the feedback.
I fixed the deep inheritance issues and updated the attribute name. But I'm not sure about your example.
If you do so, then the class Animal will not have the InheritColumns attribute, but it will still inherit properties from base classes. I think if you want to inherit properties from base classes, you need to set this attribute manually, otherwise you won't be able to disable this behavior.
What do you think?

@sveinungf
Copy link
Owner

In the example Animal doesn't have the attribute because it doesn't have a base class, only derived classes. So it doesn't have anything to inherit.

Otherwise I agree, the attribute must be there to inherit properties. If the attribute was removed from the Mammal class in the example, then the property from the Animal class would be ignored and there would only be 2 columns: CanWalk and Breed. If the attribute was removed from the Dog class, then the behavior is the same as today with Breed being the only column.

@Ashymonth
Copy link
Author

Ashymonth commented Apr 26, 2024

Yeah you're right.
The order of the columns here should be: CanWalk, DateOfBirth, Breed confused me a little bit, sorry
Then it implemented correctly in my last commits. I have added tests for those cases

@Ashymonth
Copy link
Author

Hi. Any updates ?

@sveinungf
Copy link
Owner

This looks very good, thank you!

@sveinungf sveinungf merged commit dc72b3a into sveinungf:main May 2, 2024
1 check passed
@Ashymonth Ashymonth deleted the feature/support-inherited-pproperties branch May 12, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants