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

feat(Functional): add With extension method #45

Closed
wants to merge 6 commits into from
Closed

feat(Functional): add With extension method #45

wants to merge 6 commits into from

Conversation

Odonno
Copy link
Contributor

@Odonno Odonno commented Nov 24, 2017

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
Copy link
Owner

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
Copy link
Contributor Author

Odonno commented Nov 25, 2017

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
Copy link
Contributor Author

Odonno commented Nov 26, 2017

@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
Copy link
Owner

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
Copy link
Owner

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

@DavidArno DavidArno closed this Nov 29, 2017
@Odonno
Copy link
Contributor Author

Odonno commented Nov 29, 2017

@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
Copy link
Owner

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
Copy link
Contributor Author

Odonno commented Nov 29, 2017

@DavidArno Ok. No problem.

Do you have a release date for the next version?

@DavidArno
Copy link
Owner

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
Copy link
Contributor Author

Odonno commented Nov 30, 2017

@DavidArno That sounds great.

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