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

Explicit interface order #337

Merged
merged 6 commits into from Sep 17, 2016

Conversation

Projects
None yet
2 participants
@samcragg
Contributor

samcragg commented Sep 13, 2016

Here's my attempt at fixing issue #315 and adding the ability to place explicit interface members after normal public methods.

Note I've added a dependency on NSubstitute (via NuGet) for the unit tests - I'm happy to change that to another mocking framework if you have a preference.

samcragg added some commits Sep 13, 2016

Fix sorting of explicit interface members
When sorting, interface members are now checked if they are explicit
implementations and, if so, will only use the member name when
performing a name sort (i.e. IInterface.Method will be normalized to
Method)
Add new reorganizing setting
Allows the placing of explicit interface members at the end of their
group to be specified with the options dialog
Sort explicit interface members at the end
Explicit interface members will now be sorted at the end of their group
if the setting is checked.

@codecadwallader codecadwallader added this to the v10.2 milestone Sep 14, 2016

@codecadwallader codecadwallader self-assigned this Sep 14, 2016

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Sep 14, 2016

Thanks so much for putting this together. I'd like to look over it a little more, test it out and I'm sure I'll have a few comments - but at first blush it looks really good.

I haven't used NSubstitute before.. but I've used Moq and Microsoft Fakes which are similar. NSubstitute looks pretty straight-forward to me though so seems like we should roll with it. As you may have noticed I've done very little mocking, and very heavy integration testing through the IDE. It's more realistic, but it's also extremely slow and not CI friendly - so glad to see more pure unit tests.

@samcragg

This comment has been minimized.

Contributor

samcragg commented Sep 14, 2016

Thanks for the kind comments - there's no rush as far as I'm concerned; the code in the project is in a real good shape to expand on (I was dreading figuring out how to add a setting but when I looked at the code it was very simple).

Like I said, I can swap out NSubstitute for Moq if you're more familiar with it - it's only used in one place to mock the CodeFunction2 interface, as it had far too many members to create a manual fake I figured it was easier to just use a mocking framework.

BTW, it looks like the CI server can't restore NuGet packages - since it's only the unit test project I figured it wouldn't hurt brining in an outside dependency but I'm not sure how to get the CI to do a nuget restore on it, sorry.

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Sep 15, 2016

I'm glad you found the code accessible. :) Having something like 160 settings already does make that a pretty well-worn pattern. ;)

No need to swap out NSubstitute - again it looks pretty straight-forward and much lighter than Microsoft Fakes as an example.

I'll take a look into the CI server failing the builds. We are using NuGet for the main CodeMaid project so it already knows how to restore packages, but maybe it isn't doing it across the solution.

@codecadwallader

Overall looks great, just some minor nitpicks as I'm sure you anticipated from the mind of one obsessed with whitespace.. ;)

@@ -1,7 +1,7 @@
using System.Collections.Generic;

This comment has been minimized.

@codecadwallader

codecadwallader Sep 15, 2016

Owner

Nitpick, but can you sort the using statements alphabetically? This is the default as of VS2012, even though previously System always came first and some tools like ReSharper still enforce it.

@@ -123,6 +138,20 @@ private static int CalculateConstantOffset(BaseCodeItem codeItem)
return codeItemField.IsConstant ? 0 : 1;
}
private static int CalculateExplicitInterfaceOffset(BaseCodeItem codeItem)

This comment has been minimized.

@codecadwallader

codecadwallader Sep 15, 2016

Owner

Another nitpick, but can you move this above CalculateConstantOffset so these methods appear in the order they are invoked?

{
// Try to find where the interface ends and the method starts
int dot = name.LastIndexOf('.') + 1;
if ((dot > 0) && (dot < name.Length))

This comment has been minimized.

@codecadwallader

codecadwallader Sep 15, 2016

Owner

A convention I've started using for 'between' checks is to put the variable in the middle, with the smaller check on the left and the larger check on the right. Minor readability enhancement. So for example:

if (0 < dot && dot < name.Length)

This comment has been minimized.

@samcragg

samcragg Sep 15, 2016

Contributor

I do normally as well - this one slipped by as it evolved into that (initially it was just a check for -1, but then I added to the index and then I thought I best be safe about index out of bounds)

{
string name = codeItem.Name;
var interfaceItem = codeItem as IInterfaceItem;
if ((interfaceItem != null) && interfaceItem.IsExplicitInterfaceImplementation)

This comment has been minimized.

@codecadwallader

codecadwallader Sep 15, 2016

Owner

This could be re-written as if (interfaceItem?.IsExplicitInterfaceImplementation ?? false)
Very debatable which is easier to read, so just food for thought. :)

This comment has been minimized.

@samcragg

samcragg Sep 15, 2016

Contributor

Initialially I wrote it as if (interfaceItem?.IsExplicitInterfaceImplementation == true) but didn't like the lifting to nullable and also it's (for me!) weird to have if statements explicitly comparing for true, as it looks redundant.

This comment has been minimized.

@codecadwallader

codecadwallader Sep 15, 2016

Owner

Makes sense and agree, thanks.

.ThenBy(x => Settings.Default.Reorganizing_AlphabetizeMembersOfTheSameGroup ? (object)x.Name : (object)x.StartOffset)
.ToList();
var desiredOrder = new List<BaseCodeItemElement>(currentOrder);
desiredOrder.Sort(new CodeItemTypeComparer(Settings.Default.Reorganizing_AlphabetizeMembersOfTheSameGroup));

This comment has been minimized.

@codecadwallader

codecadwallader Sep 15, 2016

Owner

This certainly looks simpler then two separate sort passes. Are you convinced it has the same effect?

This comment has been minimized.

@samcragg

samcragg Sep 15, 2016

Contributor

I'm quite confident. Although List.Sort isn't a stable sort, I don't think we'll ever return two things as equal as we use StartOffset as a tiebreaker (and if that was the same it wouldn't matter switching them around ;-) )

The ThenBy part is handling inside the comparer, as it was already there for the Spade tool. Since they use different settings the flag to switch on name sorting is passed into the constructor.

This comment has been minimized.

@codecadwallader

codecadwallader Sep 15, 2016

Owner

Cool, thank you for confirming. Agree that StartOffset should break any ties, as even variables on the same line (e.g. int x, y;) have different character offsets.

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Sep 15, 2016

Tada, build now passing. :)

samcragg added some commits Sep 15, 2016

Code review tweaks
Minor readability tweaks:
* Moved using statement so they're alphabetical
* Moved CalculateConstantOffset so it's in the same order it's called
* Improved readability of the range check for the explicit interface
member name
@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Sep 15, 2016

Thanks for the quick fixes. I'll play around with this a little locally with the plan to merge this weekend. After merging it will be auto-deployed to the CI channel so there will be a build you can install for use as well as it getting pushed out to anybody setup with a direct feed to that channel.

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Sep 17, 2016

Ran it through a couple paces and it worked well for me. Merging! :) As mentioned above, feel free to confirm it in the CI channel build. Thanks again for contributing! I hope you don't mind being accredited in the next release notes?

@codecadwallader codecadwallader merged commit e7eec28 into codecadwallader:master Sep 17, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@samcragg samcragg deleted the samcragg:explicit_interface_order branch Sep 17, 2016

@samcragg

This comment has been minimized.

Contributor

samcragg commented Sep 17, 2016

Thanks for that, just installed the CI and ran it over one of my projects that has some explicit interface methods mixed with public ones and it handles all the files now perfectly (for my OCD anyway :D )

I have no objection to my name appearing anywhere, though it's not necessary as I just wanted to give back to something I've used for years and love (it's the first thing that gets installed in VS).

Keep up the great work!

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Sep 17, 2016

Awesome, that's really nice and rewarding to hear. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment