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

Rubberduck c#6&7 update. Second half of Rubberduck folder #3571

Merged

Conversation

@IvenBach
Copy link
Member

@IvenBach IvenBach commented Nov 26, 2017

F5 and Unit tested.

This is the second half of the Rubberduck folder. Folder API through ToDoItems.

@@ -480,15 +480,21 @@ private void SetErrorState(CodeExplorerItemViewModel itemNode, QualifiedModuleNa
private void ExecuteCollapseNodes(object parameter)
{
var node = parameter as CodeExplorerItemViewModel;
if (node == null) { return; }
if (node == null)

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

can be rewritten using is like so:

if (!parameter is CodeExplorerItemviewModel node)
{
    return;
}

SwitchNodeState(node, false);
}

private void ExecuteExpandNodes(object parameter)
{
var node = parameter as CodeExplorerItemViewModel;
if (node == null) { return; }
if (node == null)

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

see above

else { return Visibility.Collapsed; }
}
}
public Visibility ExportVisibility => CanExecuteExportAllCommand == false ? Visibility.Visible : Visibility.Collapsed;

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

we have a BoolToVisibilityConverter. Should we use that one instead?

This comment has been minimized.

@IvenBach

IvenBach Nov 26, 2017
Author Member

Where is that found? I did a search for it but didn't find one.

This comment has been minimized.

@IvenBach

IvenBach Nov 26, 2017
Author Member

The logic on this one is opposite ExportAllVisibility below. Is that correct? Rewriting it without the == false leads to public Visibility ExportVisibility => CanExecuteExportAllCommand ? Visibility.Collapsed : Visibility.Visible; or am I mistaken?

This comment has been minimized.

This comment has been minimized.

@IvenBach

IvenBach Nov 28, 2017
Author Member

Can this be done on properties? Everything I understood was dealing with XAML and going through the binding engine.

This comment has been minimized.

@Vogel612

Vogel612 Nov 28, 2017
Member

you need to pass the converter to the Binding. Something like Visibility={Binding Path=ExportVisibility, Converter={StaticResource BoolToVisibilityConverter}} if I'm going by how i've seen it

else { return Visibility.Collapsed; }
}
}
public Visibility ExportAllVisibility => CanExecuteExportAllCommand ? Visibility.Visible : Visibility.Collapsed;

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

Here as well

return Projects == null || Projects.Count == 0 ? Visibility.Collapsed : Visibility.Visible;
}
}
public Visibility TreeViewVisibility => Projects == null || Projects.Count == 0 ? Visibility.Collapsed : Visibility.Visible;

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

and here, too

{
return new List<RegexSearchResult>();
}
return _search.TryGetValue(scope, out var searchFunc) ? searchFunc.Invoke(searchPattern) : new List<RegexSearchResult>();

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

one could consider breaking the line before ? and : here, to get rid of vertical scrolling

public string AllPropertyCode => string.Format("{0}{1}{2}",
GetterCode,
(GenerateLetter ? LetterCode : string.Empty),
(GenerateSetter ? SetterCode : string.Empty));

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

Could just use an interpolated string instead, right?

This comment has been minimized.

@IvenBach

IvenBach Nov 26, 2017
Author Member

It may just be me, but $"{GetterCode}{(GenerateLetter ? LetterCode : string.Empty)}{(GenerateSetter ? SetterCode : string.Empty)}" feels crowded. It seemed clearer in the current format.

@@ -32,14 +32,7 @@ public ExtractInterfacePresenter Create()
}

var model = new ExtractInterfaceModel(_state, selection.Value);
if (!model.Members.Any())
{
// don't show the UI if there's no member to extract

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

this comment explains why we return null. I'd opt to keep it

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

Do note that I dislike not showing the UI on a "user error"

This comment has been minimized.

@retailcoder

retailcoder Dec 14, 2017
Member

@Vogel612 agreed, needs a messagebox or something.


return Type == null ? signature : signature + " As " + Type;
//var signature = MemberType + " " + Member.IdentifierName + "(" +
// string.Join(", ", MemberParams) + ")";

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

commented code?

@@ -200,7 +200,7 @@ private void AddParameter(Declaration targetMethod, Declaration targetVariable,
_rewriters.Add(rewriter);

var argList = paramList.arg();
var newParameter = Tokens.ByVal + " " + targetVariable.IdentifierName + " "+ Tokens.As + " " + targetVariable.AsTypeName;
var newParameter = Tokens.ByVal + " " + targetVariable.IdentifierName + " " + Tokens.As + " " + targetVariable.AsTypeName;

This comment has been minimized.

@Vogel612

Vogel612 Nov 26, 2017
Member

string interpolation opportunity

bool CanShowSplash { get; set; }
bool CanCheckVersion { get; set; }
bool IsSmartIndenterPrompted { get; set; }
bool IsAutoSaveEnabled { get; set; }

This comment has been minimized.

@MDoerner

MDoerner Nov 29, 2017
Contributor

Why change the names of our settings? In particular CanShowSplash is misleading.

This comment has been minimized.

@IvenBach

IvenBach Dec 13, 2017
Author Member

I had seen bool with Is... and others without. The thought of standardizing them a bit came to mind. https://chat.stackexchange.com/transcript/14929?m=41324402#41324402 led me to add them. Is this something I should revert?

This comment has been minimized.

@retailcoder

retailcoder Dec 14, 2017
Member

Yes, please. That said IIRC the autosave feature is long gone, ..would removing that setting cause issues?

This comment has been minimized.

@Vogel612

Vogel612 Dec 14, 2017
Member

if it does, then those issues need to be addressed sooner or later anyways. So I'm all for removing it ...

@retailcoder retailcoder merged commit 372fdbd into rubberduck-vba:next Dec 19, 2017
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@IvenBach IvenBach deleted the IvenBach:Rubberduck.TodoItems_C#6&7_Update branch Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants