DavidArno / SuccincT

Discriminated unions, pattern matching and partial applications for C#
MIT License
267 stars 15 forks source link

feat(Functional): add With extension method #45

Closed Odonno closed 6 years ago

Odonno commented 6 years ago

Linked to #44

Based on this stackoverflow post (https://stackoverflow.com/questions/3445784/copy-the-property-values-to-another-object-with-c-sharp), I started to work on the With extension method.

I added two methods : Copy to create a news immutable object and With by reusing the previous method to create a new immutable object and adding new properties.

DavidArno commented 6 years ago

I like what you have done so far, but as it stands this will only work with mutable types. It would be useful if it could match getter properties to constructor parameters. Is that possible with reflection? I've never looked into it, so that may not be possible.

What I'm thinking is that for the following type:

class Contrived
{
    public Contrived(int a, int b) => (A, B) = (a, b);

    public int A { get };
    public int B { get };
    public int C { get; set; }
}

Then Copy and With could match property A to parameter a and B to b and populate the instance accordingly.

Odonno commented 6 years ago

Yes, my first objective was to make it work with POCO. I will write some tests with get only properties and specialized constructor and see if I can adapt the code to make it work.

Odonno commented 6 years ago

@DavidArno I updated my code based on your request. I am now using the most relevant constructor instead of the default one. The only restriction of the code is that you should named your constructor parameter correctly or it won't work.

About performance issues, I think we can drastically improve the methods by caching information we receive from TypeInfo. I will cache the necessary information in a new object so it can take a little longer at the first call of Copy/With but after that it should be way faster.

DavidArno commented 6 years ago

Not sure why this isn't showing as merged, but it is. I've added it to the v3.1 branch.

Now I just need to find time to update the docs and release. 😀

DavidArno commented 6 years ago

Closing as Github hasn't detected the merge (probably due to me doing things wrongly).

Odonno commented 6 years ago

@DavidArno Well, I think you merged it manually (with your computer) and not using the website. I think it's a bad behavior because you did merge the code but the website shows you refused it.

Moreover, it is easier to merge using the website by changing the target branch in the PR settings. https://github.com/blog/2224-change-the-base-branch-of-a-pull-request

DavidArno commented 6 years ago

Thanks for the tip. That wouldn't have worked in this case though. master was ahead of v3.1 due to poor branch management on my part, so when I merged I ended up with conflicts and it attempted to remove important files. So I had to manually merge.

Completely my fault. Apologies.

Odonno commented 6 years ago

@DavidArno Ok. No problem.

Do you have a release date for the next version?

DavidArno commented 6 years ago

I don't, unfortunately. I'm aiming for next week, but no promises.

Looking through the code, I have a suggestion. Both Copy and With return an Option<T>. As such, I'm tempted to rename them TryCopy and TryWith, respectively. I'll then create new versions of Copy and With that return T or throw an exception if they fail.

Are you happy with that?

Odonno commented 6 years ago

@DavidArno That sounds great.