search

OfficeDev / Open-Xml-PowerTools

MIT License
692 stars 26 forks source link

Refactor WmlComparer #255

Closed ThomasBarnekow closed 6 years ago

ThomasBarnekow commented 6 years ago

This commit refactors the WmlComparer, putting classes into their own files and splitting the very large WmlComparer class into multiple partial classes. If further fixes many (but not all) issues flagged by static code analysis.

For easier reference, the existing WmlComparer class is temporarily retained in the OpenXmlPowerTools.Previous namespace. For organizational purposes, the refactored version is stored in theComparer folder, which does however not create a namespace.

The objective of this commit is to not create breaking changes. The one exception is the removal of the internal WmlComparer.Coalesce method, which was only invoked in one unit test. This is deemed a left-over from previous rewrites and is thus removed. The unit test is commented out for now.

This pull request essentially prepares the WmlComparer for future enhancements.

ThomasBarnekow commented 6 years ago

@tomjebo Regarding the sln.DotSettings file, you are likely aware that this is a ReSharper-generated file for the team settings, which should generally be under source code control. This file determines the code layout and formatting applied by ReSharper. For people using ReSharper, those settings will make sure that ReSharper does not flag issues just because the settings on the local machine are different. For people not using ReSharper, those settings are completely ignored and don't hurt.

I can certainly remove it if there is another way of ensuring consistency and quality in the code base. But that doesn't exist in the moment.

Would that change your thinking?

ThomasBarnekow commented 6 years ago

@tomjebo Regarding the license and copyright info, do you mean the info concerning the WmlComperer or the other code I've contributed? For example, among other things, I wrote the following classes (in corresponding files) from scratch:

tomjebo commented 6 years ago

@ThomasBarnekow the license/copyright issue is being handled offline. Don't worry about that. We can just go ahead with your PR without considering that for now and then we'll update all the copyright header comments and the LICENSE.txt file to match with the SDK in a separate PR.

Regarding the reSharper DotSettings file, we typically use stylecop.json and just add resharper and other tools in .gitignore for folks to maintain locally. I would be ok with adding it but I don't see that it's common practice for OfficeDev projects. Let me check with some other folks and get back to you on this.

ThomasBarnekow commented 6 years ago

@tomjebo I'm happy to embrace whatever standard is used. Consistency within the OfficeDev projects would certainly add value.

tomjebo commented 6 years ago

@ThomasBarnekow we should be ok with having DotSettings (and other tool settings files) co-exist with stylecop.json. So I'm ok with adding the file. @twsouthwick do you have any issues? If not, then I'll merge.

twsouthwick commented 6 years ago

I'm fine with it for now as it helps maintain some consistency. The challenge with it is dependent on an install of Resharper (with associated license) and isn't enforced at build time. Long term, getting the stylecop analyzers in will negate the need hopefully, but since they're not in at this moment, I'm good with it. .