Change templates to be a single template. by ILMTitan · Pull Request #792 · GoogleCloudPlatform/google-cloud-visualstudio

ILMTitan · 2017-09-05T17:39:15Z

Create a single template with a version and type selection dialog.
This is a fix for issues #781 and #782.

…ction dialog.

codecov · 2017-09-06T01:01:03Z

Codecov Report

Merging #792 into master will increase coverage by 1.1%.
The diff coverage is 88.38%.

@@            Coverage Diff            @@
##           master     #792     +/-   ##
=========================================
+ Coverage   10.46%   11.57%   +1.1%     
=========================================
  Files         496      504      +8     
  Lines       12130    12299    +169     
=========================================
+ Hits         1270     1423    +153     
- Misses      10860    10876     +16

Impacted Files	Coverage Δ
...xtension/GoogleCloudExtension/Utils/StringUtils.cs	`100% <ø> (ø)`	⬆️
...udExtension/Utils/LocalizedDisplayNameAttribute.cs	`100% <ø> (ø)`	⬆️
...ateChooserDialog/TemplateChooserWindowContent.xaml	`0% <0%> (ø)`
.../CloudExplorerSources/PubSub/TopicViewModelBase.cs	`95.55% <0%> (-4.45%)`	⬇️
...ChooserDialog/TemplateChooserWindowContent.xaml.cs	`0% <0%> (ø)`
...ogs/TemplateChooserDialog/TemplateChooserWindow.cs	`0% <0%> (ø)`
...leCloudExtension/Utils/EnumDisplayNameConverter.cs	`0% <0%> (ø)`
...lateWizards/GoogleProjectTemplateSelectorWizard.cs	`100% <100%> (ø)`
...ateChooserDialog/TemplateChooserViewModelResult.cs	`100% <100%> (ø)`
...ion/TemplateWizards/GoogleProjectTemplateWizard.cs	`82.6% <100%> (-1.27%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d925e1...1213b58. Read the comment docs.

ivannaranjo · 2017-09-07T09:12:29Z

I'll go through the massive changelist to perform some spot checks, any guidance on what to expect would be great.

Also, is the feature complete? Should this be in its own branch until the templates are fully functional?

ivannaranjo

I am now convinced that this change should not go against the master branch. The feature is not complete and therefore it should go against a private branch were we can iterate freely. Please change the target of the PR.

I did not look at the templates themselves, there are waaay too many files to do an effective review there. We'll have to go one by one, generating the various combinations and looking at the generated code.

ivannaranjo · 2017-09-07T10:12:32Z

+
+namespace GoogleCloudExtension.TemplateWizards
+{
+


Empty line.

ivannaranjo · 2017-09-07T10:14:17Z

+{
+    /// <summary>
+    /// Wizard for a project template. Delegates to GoogleProjecTemplateSelectorWizard
+    /// in GoogleCloudExtension.dll. That class can not be used directly because


Could you ellaborate a bit about the issues? Open an issue in the github repo so we can track this issue and point to it.

When I first tried to reference a wizard class in GoogleCloudExtension.dll from a vstemplate file, I got an error popup about being unable to reference a class from an MEF assembly (which GoogleCloudExtension.dll, containing the package, is).

Please on an issue so we can come back to this again later and fix it, if you think that this can be eventually fixed.

ivannaranjo · 2017-09-07T10:15:51Z

+        {
+            var fromUri = new Uri(fromDirectory.EnsureEndSeparator().Replace('\\', '/'));
+            var toUri = new Uri(toDirectory.EnsureEndSeparator().Replace('\\', '/'));
+            return fromUri.MakeRelativeUri(toUri).ToString().Replace('/', Path.DirectorySeparatorChar);


You are only using Path.DirectorySeparatorChar here, but not above, you should use it everywhere in this method to be consistent.

The documentation is not exactly clear on what the relative path from one directory to another means, I would suggest adding an example.

I don't use it above, because I want to convert both types of directory separators to '/' so Uri will treat them as expected. I use it here because I am converting back to the native format.

Hmm... Perhaps a comment on what you are doing would make this code clearer to read. Also, our code for VS is mostly Windows specific (i.e. not portable) so we can always assume that \ will be the path separator.

Added comments.

ivannaranjo · 2017-09-07T10:17:46Z

+        /// return false if it is null.
+        /// Empty string is valid so it returns true.
+        /// </summary>
+        public static bool IsDigitsOnly(string text) => text != null && text.All(char.IsDigit);


Why is an empty string considered to be all digits? It should return false since by definition it does not contain digits.

Also not my code.

ivannaranjo · 2017-09-07T10:18:34Z

+        /// </summary>
+        public static IEnumerable<string> SplitStringBySpaceOrQuote(string source)
+        {
+            if (source == null)


If the source is null or empty then you should probably return an empty enumerable no?

This is not a function I wrote. This is a file being move into a different assembly.

Please open an issue so we can track fixing this code then.

ivannaranjo · 2017-09-07T10:36:18Z

+        {
+            get
+            {
+                if (_isMvc)


Seems like this should be an enum instead of booleans, since the type of template is mutually exclusive.

ivannaranjo · 2017-09-07T10:37:41Z

+                <ResourceDictionary Source="../../../Theming/ThemingResources.xaml"/>
+            </ResourceDictionary.MergedDictionaries>
+            <DataTemplate DataType="{x:Type local:FrameworkType}">
+                <TextBlock Text="{Binding Converter={utils:EnumDisplayNameConverter}}"/>


Too much magic, just have this in the view model, which is the class responsible for talking to the view. The enums should always be view agnostic, including not having the localized string attached to them.

ivannaranjo · 2017-09-07T10:39:39Z

+
+                string version = result.SelectedVersion.Version;
+                string templatePath = Path.Combine(
+                    thisTemplateDirectory, "..", "..", "..", result.AppType, "1033", version, $"{version}.vstemplate");


I applaud the effort, but the code is Windows specific. You can specify a format string that will create the right path without using this. Is ok to hard code the '' for file paths on Windows specific code.

Changes this to use Path.DirectoryName multiple times.

ivannaranjo · 2017-09-07T10:40:26Z

+        /// <inheritdoc/>
+        public void RunFinished()
+        {
+            throw new NotImplementedException("This wizard should delegate to another wizard and cancel itself.");


This tells me that this PR should go against a separate feature branch, not the master branch, as we should not be committing unfinished features there.

This is not unfinished. This method should never be called.

ivannaranjo · 2017-09-07T10:43:11Z

+
+namespace GoogleCloudExtension.Utils
+{
+    public class EnumDisplayNameConverter : MarkupExtension, IValueConverter


Please don't use this, enums should not have a localized display string associated with them.

ivannaranjo · 2017-09-12T15:19:47Z

+        /// <summary>
+        /// The selected framework type.
+        /// </summary>
+        public FrameworkType SelectedFramework { get; }


I'm now even more convinced that we should not have localized strings associated with the enums like this. We should have a small class that associates the enum with the localized string, including one with the default value if necessary.

ivannaranjo · 2017-09-12T15:20:34Z

+    public class TemplateChooserViewModel : ViewModelBase
+    {
+        private string _gcpProjectId;
+        private FrameworkType _selectedFramework = (FrameworkType)(-1);


You should instead define an enum value of None (which should be set to 0 btw) to indicate that there's no framework selected. This is one of the reasons why having magic to associate strings with enums does not work.

Add Asp.Net Core 1.1 templates. Add unit tests. Enable ProjectTemplate.Tests with VS2017.

ivannaranjo

Comments about over complicated XAML constructs.

Also, make sure that the .csproj are idiomatic. For the 2.0 case Microsoft made a big case of making it very very simple, we should follow suit and have the same idiomatic projects.

ivannaranjo · 2017-09-14T09:46:40Z

+    /// </summary>
+    public enum AppType
+    {
+        None,


Nit, have a small comment for each option, helps when using it in intellisense. They are self-explanatory I know, but it will help in the long run.

ivannaranjo · 2017-09-14T09:47:37Z

+
        private string _gcpProjectId;
-        private FrameworkType _selectedFramework = (FrameworkType)(-1);
+        private FrameworkType _selectedFramework = FrameworkType.None;


Great, thank you! :)

ivannaranjo · 2017-09-14T09:51:20Z

                        GroupName="AppType"
                        Content="{x:Static ext:Resources.WizardTemplateChooserMvcLabel}"
-                        IsChecked="{Binding IsMvc}"/>
+                        IsChecked="{Binding AppType, Converter={utils:RadioEnumConverter Target={x:Static local:AppType.Mvc}}}"/>


I have to ask, what is the advantage of this vs. what you had before? Since the set of choices is well known, and limited, why having the extra complexity that your solution adds? If we were to have 10s of choices I would agree, but we only have 2 choices, and I don't think that we will ever have more than 4. Having it hardcoded in the UI is perfectly valid.

Changed this back to have IsMvc/IsWebApi properties on the ViewModel.

ivannaranjo · 2017-09-14T09:53:51Z

            </ResourceDictionary.MergedDictionaries>
            <DataTemplate DataType="{x:Type local:FrameworkType}">
-                <TextBlock Text="{Binding Converter={utils:EnumDisplayNameConverter}}"/>
+                <TextBlock x:Name="TextDisplay"/>


Wouldn't it be simpler to just have two entries under the ComboBox with the two options? I'm sorry but this solution is waaay more complex than what it really needs to be. You can even use the Tag property in the TextBlock to contain the value of the enum that you want and just add two TextBlock instances.

ivannaranjo · 2017-09-14T09:57:04Z

  <ItemGroup>
    <PackageReference Include="Google.Cloud.Diagnostics.AspNetCore" Version="1.0.0" />
-    <PackageReference Include="Microsoft.AspNetCore.All" Version="2.0.0" />
+    <PackageReference Include="Microsoft.AspNetCore" Version="2.0.0" />


Looking at a 2.0 project generated by dotnet, it is much much simpler, it collapses all of these packages into:

<ItemGroup> <PackageReference Include="Microsoft.AspNetCore.All" Version="2.0.0" /> </ItemGroup>

We should make sure that the generated .csproj files are idiomatic.

Change framework type enum combobox to use ComboBoxItems. Change ASP.NET Core 2.0 templates to use Microsoft.AspNetCore.All.

Move EnsureEndSeparator to PathUtils. Add unit tests.

ivannaranjo

Really looking good, thank you for taking the feedback.

Change templates to be a single template with a version and type sele…

fb4073c

…ction dialog.

ILMTitan self-assigned this Sep 5, 2017

ILMTitan requested review from Deren-Liao and ivannaranjo September 5, 2017 17:39

ILMTitan added the templates label Sep 5, 2017

Jim Przybylinski added 4 commits September 5, 2017 10:39

Fix comments and variable names.

afd4ef6

Fix AssemblyInfo.

736e0d1

Fix unextracted Features label string.

3e43bf9

Remove unused resources. Change FrameworkType to an enum.

f7659f4

ivannaranjo suggested changes Sep 7, 2017

View reviewed changes

ivannaranjo suggested changes Sep 12, 2017

View reviewed changes

Jim Przybylinski added 2 commits September 13, 2017 14:27

Resolve PR Comments.

8a6fc6e

Add Asp.Net Core 1.1 templates. Add unit tests. Enable ProjectTemplate.Tests with VS2017.

Fix compile error.

31826e9

ivannaranjo suggested changes Sep 14, 2017

View reviewed changes

Jim Przybylinski added 3 commits September 14, 2017 11:25

Re-add IsMvc and IsWebApi properites.

b45a547

Change framework type enum combobox to use ComboBoxItems. Change ASP.NET Core 2.0 templates to use Microsoft.AspNetCore.All.

Move StringUtils back to GoogleCloudExtension.

472a209

Move EnsureEndSeparator to PathUtils. Add unit tests.

Add comments to AppType.

1213b58

ivannaranjo approved these changes Sep 15, 2017

View reviewed changes

ILMTitan merged commit 7e98c58 into GoogleCloudPlatform:master Sep 15, 2017

ILMTitan deleted the single-template-dialog branch September 15, 2017 18:45

This was referenced Sep 15, 2017

Add template support for .NET Core 1.1 #635

Closed

Skip button text is cut off #789

Closed


		namespace GoogleCloudExtension.TemplateWizards
		{

Uh oh!

Conversation

ILMTitan commented Sep 5, 2017

Uh oh!

codecov Bot commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ivannaranjo commented Sep 7, 2017

Uh oh!

ivannaranjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ILMTitan Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivannaranjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 6, 2017 •

edited

Loading

ILMTitan Sep 12, 2017 •

edited

Loading