Improve of Fluent API

[WojciechNagorski]

I can't configure may serializer in this way:

return new ConfigurationContainer();
.EnableDeferredReferences();
.Type<IElement>().EnableReferences(p => p.Id);
.Type<Section>().EnableReferences(p => p.Id);
...
.Type<Building>().EnableReferences(p => p.Id)
.Create();

I have to do like below. Without use Fluent API.

var config = new ConfigurationContainer();
config.EnableDeferredReferences();
config.Type<IElement>().EnableReferences(p => p.Id);
config.Type<Section>().EnableReferences(p => p.Id);
...
config.Type<Building>().EnableReferences(p => p.Id);

return config.Create();

[WojciechNagorski]

new ConfigurationContainer()
  .Type<Section>()
    .Member(p=>p.IsSelected).Name("Selected")
    .Member(p => p.IsEmpty).Name("Empty").Create();

It also does not work.

[Mike-E-angelo]

Alright, for this one, as mentioned Fluent API isn't perfect. However, there is an overload for Member to pass in a configuration delegate. There wasn't one for Type, so I just added it and provided examples here:

https://github.com/wojtpl2/ExtendedXmlSerializer/blob/v2.0.0/test/ExtendedXmlSerializer.Tests/ReportedIssues/Issue132Tests.cs

Hope that is sufficient for now. 👼

[WojciechNagorski]

Honestly it is not good solution. We should do this in this version becase it is important for user. We are on the second place.

I tried do this today and I thing it doesn't seem very hard.

TypeConfiguration should inhered from ConfigurationContainer and MemberConfiguration<T, TMember> should inhered from TypeConfiguration

Or we can expose my fluent API as proxy with your interall API.

[Mike-E-angelo]

Ahhh since I've already tried twice now to address the fluent API, and it is still not sufficient, I have no problem with you taking over the story to make sure it is done right. :) I am not the happiest with it, either... so, feel free to fix it if you have a better idea of how it should be done. 👍

[Mike-E-angelo]

So I am thinking of this more. Would this be OK for v2?

new ConfigurationContainer().EnableDeferredReferences()
.Type<IElement>()
.EnableReferences(p => p.Id).Root
.Type<Section>().EnableReferences(p => p.Id).Root
.Type<Building>().EnableReferences(p => p.Id)
.Create();

Essentially making the IRootContext the type container instead of the IConfigurationContainer.

[Mike-E-angelo]

Actually, if we do that, I could add an extension method to get rid of having to do .Root as well. Lemme do that real quick and see how you like it. :)

[Mike-E-angelo]

Alright... check it out now. I made it a little better. Not perfect, but hopefully it will suffice for v2. Again if you can think of a better way, please feel free to do so.

[WojciechNagorski]

I did fluent api in your code :sunglasses: But I need about one week to improve it.

[Mike-E-angelo]

Haha! Very good, then. Should have done that sooner hehe. At the very least we have a backup plan here in the meantime.

[WojciechNagorski]

I've pushed first version of improvement of API. I would like to improve it but currently it works 😄 You can look. I'm open to comments and suggestions.

There is documentation for FluentAPI: https://github.com/wojtpl2/ExtendedXmlSerializer/tree/FluentAPI#fluent-api

There is a real example: https://github.com/wojtpl2/ExtendedXmlSerializer/blob/FluentAPI/samples/ExtendedXmlSerializer.Samples/FluentApi/FluentApiSamples.cs#L22-L32

[WojciechNagorski]

In code you can find: ((ISource<MemberInfo>)@this).Get()

It is to change. MemberConfiguration inherit also from ISource<MemberInfo> and it have Get() method. TypeConfiguration inherit also from ISource<TypeInfo> and it have also Get() method.

Currently MemberConfiguration inherit form TypeConfiguration and it contains two implementation of Get() method. It is the reason way I need this cast.

It is to improve.

[Mike-E-angelo]

Awesome! That looks great! Let me get latest and see if I can smooth that over.

[Mike-E-angelo]

Alright... I like what you did. :) I did some adjustments around the class hierarchy as some inheritance was not necessary. Things are a bit cleaner now and all tests are passing. FWIW I did try going down the same path as you did, but got stuck with all of the generics. Making use of the AsInternal was the key trick. 👍

Anyways, check it out and see what you think now.

[Mike-E-angelo]

Reading over this again, I see that you want to have a class hierarchy like this:

IConfigurationContainer ITypeConfiguration : IConfigurationContainer IMemberConfiguration : ITypeConfiguration

I did remove this chain, not because I don't think that is a good idea, but mostly out of confusion. We can put it back in if you'd like. From my take, I would like to keep the amount of inheritance as small as possible (ala Convention over inheritance). But in this case, I thought it was TypeConfiguration inheriting from MemberConfiguration, which seems backwards. But I was wrong in my understanding here (rushed through it without thinking how you wanted it that way), so sorry to stomp on your code. 😞 We can put that back if you'd like, just let me know.

Also, IConfigurationContainer is meant to be for the top-level ConfigurationContainer from which all configuration happens... there should only be one implementation of that, which is why I removed that as well.

[WojciechNagorski]

In my solution I can do this:

            var serializer = new ConfigurationContainer()

                .Type<Person>() // Configuration of Person class
                    .Member(p => p.Password) // First member
                        .Name("P")
                        .Encrypt()
                    .Member(p => p.Name) // Second member
                        .Name("T")
                .UseEncryptionAlgorithm(new CustomEncryption())
                .Type<TestClass>() // Configuration of another class
                    .CustomSerializer(new TestClassSerializer())
                .Create();

In your don't.

I think that hierarchy is very important. IConfigurationContainer ITypeConfiguration : IConfigurationContainer IMemberConfiguration : ITypeConfiguration

It means that 'ITypeConfiguration' have all functions (extensions) from 'IConfigurationContainer'. 'IMemberConfiguration' have all functions (extensions) from 'ITypeConfiguration'. For me adding all extensions from eg 'ITypeConfiguration' to 'IMemberConfiguration' is duplicating code and antipatterns for this requirements.

Object TypeConfiguration don't have to inherit from ConfigurationContainer.

[Mike-E-angelo]

Ah... yeah, that's a good point. The reason the above doesn't work is that the extension method UseEncryptionAlgorithm expects an IConfigurationContainer when it could be an IContext. I have fixed this and committed it so that this works now.

Also, I think we are almost there with hierarchy. The other issue is that IContext does a lot of what you are talking about already. Maybe rename this to IMetadataContext? That way you have:

IConfigurationContainer ITypeConfiguration : IMetadataContext IMemberConfiguration : IMetadataContext

I guess what I was wrestling with is adding another level of hierarchy, when what is already there pretty much does the same thing. The other thing I am also trying to wrap my head around is IMemberConfiguration inheriting from ITypeConfiguration. Members are properties of types, they do not inherit from types. But they are both metadata components, which is why I suggest IMetadataContext. That way both types and members inherit all functionality from IMetadataContext (or IContext as it currently is in the code).

I am also not a fan of the extension methods. I fixed it a little bit with the 2nd pass, but it still needs work. They hierarchy did change a bit so a lot of the extension methods could look like they are not compatible, but they just need to be updated to do what I did above.

[WojciechNagorski]

It isn't good yet. Currently it works:

var serializer = new ConfigurationContainer()
    .Type<Person>()
    .OnlyConfiguredProperties()
        .Member(p=>p.Name).Identity()
    .Create();

But this doesn't work:

var serializer = new ConfigurationContainer()
    .Type<Person>()
        .Member(p=>p.Name).Identity()
    .OnlyConfiguredProperties()
    .Create();

OnlyConfiguredProperties it is only example. All Extension for type should work for member.

In my opinion, the easiest way is to do it is create hierarchy: IMemberConfiguration<T, TMember> : ITypeConfiguration<T>

[Mike-E-angelo]

Ah... no you are correct, there is a missing element here. As I mentioned earlier, I have a problem with inheriting IMemberConfiguration from ITypeConfiguration (think MemberInfo : TypeInfo in reflection... doesn't make sense, it's the other way around). However, they both do need to have access to the owning/parent type's members (or other information). This is the same as .NET's reflection MemberInfo.

We also had an ITypeConfigurationContext that I didn't like very much that kinda confused things.

So what I did was:

• Created an IInternalConfiguration, from which both IInternalTypeConfiguration and IInternalMemberConfiguration inherit from.
• Created an IMetadataContext that both ITypeConfiguration and IMemberConfiguration inherit from.
• Removed ITypeConfigurationContext and replaced it with IMemberConfigurations from which any IInternalConfiguration can access.
• Cleaned up some other issues.

So now check it out and see how you like it. :) If there are any other issues you find you cannot do, please let me know and I will fix it.

[WojciechNagorski]

Good idea with `IMetadataContext'. It looks good but I need more time. I'll check it tomorrow.

[WojciechNagorski]

It still doesn't work.

//Work
new ConfigurationContainer().Type<Person>()
    .AddMigration(element => {})
    .OnlyConfiguredProperties()
    .Create();

//Don't work
new ConfigurationContainer().Type<Person>()
    .OnlyConfiguredProperties()
    .AddMigration(element => { })
    .Create();

//Work
new ConfigurationContainer()
    .Type<Person>()
    .Member(p => p.Name).Identity()
    .OnlyConfiguredProperties()
    .Create();

//Don't work
new ConfigurationContainer()
    .Type<Person>()
    .OnlyConfiguredProperties()
    .Member(p => p.Name).Identity()
    .Create();

I think that you to mach think about patterns and best practises. It is for us. Not we for it. I agree that MemberInfo shouldn't inherit form TypeInfo in reflection. But we design API to configuration. We don't think about Member and Type but about MemberConfiguration and TypeConfiguration. In this case MemberConfiguration have to have interface TypeConfiguration.

"Done Is Better Than Perfect"

We had working version 2 days ago.

I wanted to hide from the user all things from IContext (Root and Context). Now you can do:

    .AddMigration(element => {})
    .OnlyConfiguredProperties().Parent.Root.Parent.Parent.Root
    .Create();

[WojciechNagorski]

I pushed failing test to repo.

[Mike-E-angelo]

Ahhh OK! Not a problem. All good points. I have no problem rolling back to e72511a59ea181e0bdbbae0c667149211e6a7fc4 and keeping the existing tests to make sure that they continue to work, too. :) Is that something that is easy to do? I would do it but my git is not too good. It will take me a while to figure out, and I've already made a mess as it is. ;)

[WojciechNagorski]

but if you want you can try fixing it. You don't have to revert your changes if you know that you can fix it.

On Oct 27, 2017 10:25, "Mike-EEE" notifications@github.com wrote:

Ahhh OK! Not a problem. All good points. I have no problem rolling back to e72511a https://github.com/wojtpl2/ExtendedXmlSerializer/commit/e72511a59ea181e0bdbbae0c667149211e6a7fc4 and keeping the existing tests to make sure that they continue to work, too. :) Is that something that is easy to do? I would do it but my git is not too good. It will take me a while to figure out, and I've already made a mess as it is. ;)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wojtpl2/ExtendedXmlSerializer/issues/132#issuecomment-339907164, or mute the thread https://github.com/notifications/unsubscribe-auth/AQh-j-kKLdoblO8Gx-ordl70Iee_4eEWks5swZOPgaJpZM4P-63X .

[Mike-E-angelo]

Haha yeah I can definitely look into it. An old friend used to always say "the greatest feature of any product is existence." Or "Done is better than Perfect"... same difference. :) I will see if I can address the issues above and if not, then we can revert.

Also, I agree that Parent.Root.Parent.Parent.Root is terrible. I will see if I can internalize this instead.

[WojciechNagorski]

'Parent.Root.Parent.Parent.Root' In my code didn't working too. I wanted to move it to "internal" interface.

[WojciechNagorski]

Hi. Did you have time to look into it ? If we do it, I'll publish v2.0.0.

[Mike-E-angelo]

Ahhh sorry this weekend was a busy one for me. I have this on my plate for this morning, and end of today at the latest. 👍

[Mike-E-angelo]

Alright... I fixed the extension methods, but the IContext hierarchy is really involved. :( I could probably fix it but it would take some time. I'm more apt to get v2 out the door and live with the blemish (and fix it for another version) than try to spend more time around it.

If you do find more problems with the extension methods, please let me know. I think we should get that addressed for sure.

[WojciechNagorski]

Still it is not good solution.

//Work
            new ConfigurationContainer().Type<Person>()
                .CustomSerializer((writer, person) => { }, element => new Person())
                .Member(p => p.Name).Identity();
//don't work
            new ConfigurationContainer().Type<Person>()
                .Member(p => p.Name).Identity()
                .CustomSerializer((writer, person) => { }, element => new Person());
// work
            new ConfigurationContainer().Type<Person>()
                .AddMigration(element => { })
                .Member(p => p.Name).Identity();
//don't work
            new ConfigurationContainer().Type<Person>()
                .Member(p => p.Name)
                .Identity()
                .AddMigration(element => {});

I don't like your solution. The better way is inheritance: IConfigurationContainer ITypeConfiguration : IConfigurationContainer IMemberConfiguration : ITypeConfiguration Than you don't need duplicate code like your approach. This: https://github.com/wojtpl2/ExtendedXmlSerializer/blob/acea32c3ba555193a63222da934e8232b6d6a1af/src/ExtendedXmlSerializer/ExtensionModel/Content/Extensions.cs#L93 is a duplicate of this: https://github.com/wojtpl2/ExtendedXmlSerializer/blob/acea32c3ba555193a63222da934e8232b6d6a1af/src/ExtendedXmlSerializer/ExtensionModel/Content/Extensions.cs#L78

And you have to duplicate code for each extension for ITypeConfiguration<T> type, like AddMigration(), CustomSerializer(), ...

This is proof that IMemberConfiguration is ITypeConfiguration.

Public API is the most important things. And we have to publish good version. We had a working version of the API. I know it wasn't perfect but it was good enough.

[Mike-E-angelo]

Yeah no doubt it is not perfect, just trying to get to Done. ;) I feel I have put enough time into it at this point. Let's switch back to https://github.com/wojtpl2/ExtendedXmlSerializer/commit/e72511a59ea181e0bdbbae0c667149211e6a7fc4 but keep the tests intact?

[WojciechNagorski]

We can save your work on another branch and let's swich back to previous changes with keep the tests. I hope the tests will passed ;) If everything will be ok we can publish version 2.0. What do you think? It will be good ?

2017-10-31 11:05 GMT+01:00 Mike-EEE notifications@github.com:

Yeah no doubt it is not perfect, just trying to get to Done. ;) I feel I have put enough time into it at this point. Let's switch back to e72511a https://github.com/wojtpl2/ExtendedXmlSerializer/commit/e72511a59ea181e0bdbbae0c667149211e6a7fc4 but keep the tests intact?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/wojtpl2/ExtendedXmlSerializer/issues/132#issuecomment-340715560, or mute the thread https://github.com/notifications/unsubscribe-auth/AQh-j-mZW4CZdzISqkuUf2nE_P0kwYw9ks5sxvDSgaJpZM4P-63X .

[Mike-E-angelo]

Yeah, that sounds good to me! I am no Fluent API expert so should have just left it alone. I will never learn! Haha. But yeah, we don't have to worry about saving the work. I think we should do another complete refresh/rewrite for v2.5 (or v3). Just make sure I steer clear of it. ;)

(Also, I have the work saved here in my own local branch. I don't want to pollute your repo w/ unnecessary branches.)

[WojciechNagorski]

Can you revert your changes and marge FluentAPI branche to v2.0.0 branch?

[Mike-E-angelo]

Oh! My bad... yeah I thought you were going to do that. I am not too good with Git, but I can give it a shot. It might take me a little while, though.

[Mike-E-angelo]

OK I created a new branch from e72511a59ea181e0bdbbae0c667149211e6a7fc4, merged in the tests, and pushed it as ConfigurationAPI. There appears to be errors though, so maybe I didn't do it correctly? It has been pushed so please take a look at it to see if you can address it.

[WojciechNagorski]

Ok I'll check. Thanks!

[WojciechNagorski]

I fixed it. I did not merge it with v2.0.0 because I want to improve it a bit.

[WojciechNagorski]

There is commit https://github.com/wojtpl2/ExtendedXmlSerializer/commit/6396fb985128c5555dc6b0260965d78a13370831

[Mike-E-angelo]

Ok great. The fluent API is all yours now. :)

[WojciechNagorski]

Ok

[Mike-E-angelo]

This should be addressed for now. For next version we will open a new issue for this story.