Skip to content

Conversation

Hosch250
Copy link
Member

I don't like how these three commands each implement an identical GetDeclaration and GetFolder.

@Hosch250 Hosch250 requested a review from retailcoder March 17, 2017 04:58
return ((ICodeExplorerDeclarationViewModel) node)?.Declaration;
}

private string GetFolder(object parameter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done 👍


protected override void ExecuteImpl(object parameter)
{
var folderAnnotation = $"'@Folder(\"{GetFolder(parameter)}\")";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. Should we have a "default folder" setting? That way it wouldn't need to be duplicated in every module-creating command...


protected override void ExecuteImpl(object parameter)
{
var folderAnnotation = $"'@Folder(\"{GetFolder(parameter)}\")";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to figure out a way to have this work when the module is added via standard VBE commands too... somehow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooks should do it.

}

return node == null ? null : ((ICodeExplorerDeclarationViewModel)node).Declaration;
return ((ICodeExplorerDeclarationViewModel) node)?.Declaration;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps extract some ModuleMaker type that the commands would use for this? That way the relatively complex logic below would only be in one place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

@retailcoder retailcoder merged commit a555c99 into rubberduck-vba:next Mar 17, 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
Development

Successfully merging this pull request may close these issues.

2 participants