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

DeepMerger only works for nested collections #17

Closed
kiwidude68 opened this issue Nov 9, 2016 · 8 comments
Closed

DeepMerger only works for nested collections #17

kiwidude68 opened this issue Nov 9, 2016 · 8 comments

Comments

@kiwidude68
Copy link

kiwidude68 commented Nov 9, 2016

Downloaded the latest source. The unit tests around it are not great, or my understanding of what it is capable of doing is incorrect.

The issue is if I have a collection element directly as a child of the section element, then DeepMerger doesn't work. You can see this for yourself with the and elements within the test config files. If you change the GetFiles method to only return one config file (config file 1) instead of all 4 in the yields - those related tests still pass?

If it was correctly merging those collections, then the result of merging test files 1-4 for the ClearCollection element should leave you with ele children with names test1, test2, test3. Instead you get test1,test2,test4 - i.e. the contents of the first test file?

To be clear about what I want to achieve - I simply want to be able to have:
File 1:

<mySection>
  <myObjects>
    <myObject name="Foo" />
  </myObjects>
</mySection>

and File2:

<mySection>
  <myObjects>
    <myObject name="Bar" />
  </myObjects>
</mySection>

Merging should result in being the same as:

<mySection>
  <myObjects>
    <myObject name="Foo" />
    <myObject name="Bar" />
  </myObjects>
</mySection>

An additional problem with DeepMerger - it doesn't work with App.config files. So in my example above, if File 1 is my App.config, and File 2 is MyCustom.config, and I attempt to merge with UsingFile("MyCustom.config") then the DeepMerger throws an exception saying that the configuration is read-only. UPDATE - I solved this by overriding IsReadOnly on the ConfigurationElement.

Finally yet another issue with it is it doesn't support the element, just gets ignored.

@Yegoroff
Copy link
Owner

thanks for highlighting these issues, I'll look on them and provide fixes.

@kiwidude68
Copy link
Author

kiwidude68 commented Nov 11, 2016

Hi mate, thanks for the reply. With a lot of experimentation I got something running in the end to do what I needed. I'm not quite sure why I had so many problems getting it to work, it did take quite a few hours, me being a numpty I guess and the examples not being quite what I was after.

In the end I think perhaps the only thing of the above that I didn't get resolved was the remove element not appearing to work (and those unit tests not seeming to make sense).

The other issue I had was (for a few months more) having to support .NET 3.5 only, so I had to make a few minor tweaks to the implementation to include it and compile the .NET 3.5 project. As per the separate issue I raised about strong naming we are having to do our own compile anyways but would be nice to get on an official package at some point.

Thanks again for your work on this.

@HeikoStudt
Copy link
Contributor

I did not subscribe to the feed, sorry for the late reply.
It is strange that the unit tests are not working as expected. I will try to look into your problems later.
The screenshots of your original issue seem to be lost?

MFG (sincerly)
HeikoStudt

@HeikoStudt
Copy link
Contributor

I've tried to reproduce your unit test behavior.

First, I had to install NUnit TestAdapter Nuget package for supporting Visual Studio 2017. I will create a Pull Request later.

Second, I had to change the nuget.config file to contain instead of .

Third, the test "Should_convert_passed_relative_paths_to_absolute_correctly" fails always. I am not quite sure why it might have changed. It fails in VS 2015 as well.

@Yegoroff Do you know why? What Visual Studio version do you use? Could we move upwards to VS 2017? Though it has its bugs/crashes, the new project structure allows for easy nuget package generation including multi-targeting.

Now, for the problem. :-)

If I comment the other files:

    private IEnumerable<FileInfo> GetFiles() {
            yield return new FileInfo(CONFIG_1);
            /*yield return new FileInfo(CONFIG_2);
            yield return new FileInfo(CONFIG_3);
            yield return new FileInfo(CONFIG_4);*/
        }

most of the unit tests are failing. The only ones going through are DeepMerger_MergeCollections_RemoveCollection and DeepMerger_MergeCollections_ClearCollection. If I understood correctly, you had the same behavior?

This is the case as you cannot test whether a non-existing element was removed/cleared or was never added. As the File1 contains the (and some adds), the end result must be exactly the File1 elements. For the remove I did not reread the details, but it holds the same fundamental idea.

As I cannot see the screenshots, I am currently not quite sure where the issue is. Can you please send me those or clearify otherwise?

MFG (sincerly)
Heiko Studt

@HeikoStudt
Copy link
Contributor

As I've seen now, those were no screenshots but code having < and > which are apparently removed by GitHub. My clear-Tag in my answer above was removed as well.
@kiwidude68 Please add those three blocks (File1, File2 and Expected) again via "Insert Code". Thanks!

MFG (sincerly)
HeikoStudt

@HeikoStudt
Copy link
Contributor

The IsReadOnly issue is in my opinion not a DeepMerger issue in itself, however DeepMerger writes in one configuration the merged data:

//A possible problem may be that we are overriding an element of a different config. for unmerging this is probably wrong //However, NConfig does not require us to be able to unmerge... var item = configs.First();

The above code in DeepMerger.cs could try to use reflection for creating a new element to merge into. For the moment, I did not investigate further, whether this is feasible at all.

MFG (sincerly)
HeikoStudt

@HeikoStudt
Copy link
Contributor

Finally yet another issue with it is it doesn't support the element, just gets ignored.
@kiwidude68 Again, please add the mentioned element as code or without < and >. Thanks.

MFG (sincerly)
HeikoStudt

@Yegoroff
Copy link
Owner

the latest commit backward compatibility of proj files, add new files to 3.5 project contains fixes for above mentioned issues with unit tests and .Net 3.5
As for the other reported cases, it looks like @kiwidude68 solved them.

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

No branches or pull requests

3 participants