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

Order by access modifier now configurable. #359

Merged
merged 7 commits into from Mar 14, 2017

Conversation

Projects
None yet
2 participants
@ahalassy
Contributor

ahalassy commented Oct 25, 2016

No description provided.

Adam Halassy and others added some commits Oct 25, 2016

Adam Halassy
[mod] Ordering by access level now can be configured
Signed-off-by: Adam Halassy <ahalassy@cognex.com>
Merge pull request #1 from ahalassy/features/order-by-visibility
[mod] Ordering by access level now can be configured
@codecadwallader

Thanks again for putting these changes together. I've got a couple questions and a handful of nitpicks (as one would hopefully expect from a code cleaning project ;)), but overall looks good!

case vsCMAccess.vsCMAccessPrivate: return 6;
default: return 0;
}
var itemsOrder = new[] {

This comment has been minimized.

@codecadwallader

codecadwallader Oct 27, 2016

Owner

Instead of starting in arrays, and converting to list.. why not just use lists?

vsCMAccess.vsCMAccessProtected,
vsCMAccess.vsCMAccessPrivate
};
if (!Settings.Default.Reorganizing_RevertAccessLevel)

This comment has been minimized.

@codecadwallader

codecadwallader Oct 27, 2016

Owner

Revert isn't a clear verb to me (means go back to an original state). I think you mean Reverse access level. In that case though, wouldn't this be a positive case where only if the flag was set would the reversal happen?

if (!Settings.Default.Reorganizing_RevertAccessLevel)
itemsOrder = itemsOrder.Reverse().ToArray();
return itemsOrder.Contains(codeItemElement.Access)

This comment has been minimized.

@codecadwallader

codecadwallader Oct 27, 2016

Owner

IndexOf always returns -1 when no match is found, so you could remove the conditional and always return IndexOf+1.

@@ -15,6 +15,18 @@
<RadioButton Content="type then access" IsChecked="{Binding PrimaryOrderByAccessLevel, Converter={x:Static cnv:BooleanInverseConverter.Default}}" />
<RadioButton Content="access then type" IsChecked="{Binding PrimaryOrderByAccessLevel}" />
</StackPanel>
<StackPanel

This comment has been minimized.

@codecadwallader

codecadwallader Oct 27, 2016

Owner

More nitpicks:

  • Can we follow the formatting conventions of the rest of the file (e.g. attributes on same line)
  • Can we use the same verbiage as other options.. (e.g. "Access levels should be ordered by" with the default option being first "public to private" and the alternate option being second "private to public")
@@ -25,7 +25,8 @@ public ReorganizingGeneralViewModel(CodeMaidPackage package, Settings activeSett
new SettingToOptionMapping<bool, bool>(x => ActiveSettings.Reorganizing_KeepMembersWithinRegions, x => KeepMembersWithinRegions),
new SettingToOptionMapping<int, AskYesNo>(x => ActiveSettings.Reorganizing_PerformWhenPreprocessorConditionals, x => PerformWhenPreprocessorConditionals),
new SettingToOptionMapping<bool, bool>(x => ActiveSettings.Reorganizing_PrimaryOrderByAccessLevel, x => PrimaryOrderByAccessLevel),
new SettingToOptionMapping<bool, bool>(x => ActiveSettings.Reorganizing_RunAtStartOfCleanup, x => RunAtStartOfCleanup)
new SettingToOptionMapping<bool, bool>(x => ActiveSettings.Reorganizing_RunAtStartOfCleanup, x => RunAtStartOfCleanup),
new SettingToOptionMapping<bool, bool>(x => ActiveSettings.Reorganizing_RevertAccessLevel, x => RevertOrderByAccessLevel)

This comment has been minimized.

@codecadwallader

codecadwallader Oct 27, 2016

Owner

Will you please move this up to continue the alphabetical ordering.

/// <value>
/// <c>true</c> if [revert order by access level]; otherwise, <c>false</c>.
/// </value>
public bool RevertOrderByAccessLevel

This comment has been minimized.

@codecadwallader

codecadwallader Oct 27, 2016

Owner

Will you please move this up to continue the alphabetical ordering. It would also be helpful to match the comment style to the others vs. auto-generated.

Perhaps a clearer name for this property would be ReverseAccessLevelOrdering

@ahalassy

This comment has been minimized.

Contributor

ahalassy commented Oct 27, 2016

Thanks for all the notes I am going to fix the code keeping this in my mind. Also, I noticed that the code ordering puts the static members to the end of the code however I need the opposite ordering in my codes. Anyway, I also implemented configuration for for static member ordering in my fork. I am going to review that taking in consideration your notes and add those changes to this pull request within a few days. Do you agree or should I handle this on a different request?

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Oct 27, 2016

Sounds good. It's pretty closely related so I'm OK with it being on the same pull request. You may already be thinking it but it sounds like a separate configuration option to reverse static members.

@ahalassy

This comment has been minimized.

Contributor

ahalassy commented Oct 27, 2016

It definitely is a separate configuration option with it's own GUI option and setting field.

ahalassy added some commits Mar 13, 2017

Merge branch 'master' of https://github.com/codecadwallader/codemaid
# Conflicts:
#	CodeMaid/Properties/Settings.Designer.cs

Signed-off-by: Adam Halassy <adam.halassy@gmail.com>
@ahalassy

This comment has been minimized.

Contributor

ahalassy commented Mar 13, 2017

Hi! Sorry for the long delay, I was quite busy during the last few months. I made several changes to align with your conventions. Can you have a look?

@codecadwallader codecadwallader merged commit 6b710b7 into codecadwallader:master Mar 14, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Mar 14, 2017

Looks good to me! Merging in to master now. :) Thanks again for your contribution! I hope you don't mind if I credit you during the next release?

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Mar 15, 2017

I made a couple formatting tweaks.. and I'm questioning the reversal logic now. The setting is off by default (i.e. public -> private).. yet in CodeItemTypeComparer it is reversing the items order when the setting is not true (i.e. off). Should the exclamation mark be removed, or am I looked at it backwards?

@ahalassy

This comment has been minimized.

Contributor

ahalassy commented Mar 15, 2017

I need to check this. Previously I planned to create a similar enhancement for static-instance level ordering - I am going to implement this. I'll check this as well, and I'll inform you if further alignments needed. My original intention was to keep your logic as default.

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Mar 15, 2017

Ok, sounds good. I went ahead and flipped that statement as I'm pretty sure from tracing through the logic it was inverted. Thanks for taking a look!

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Mar 23, 2017

I did retest that logic and confirm it's correct now.

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