-
Notifications
You must be signed in to change notification settings - Fork 359
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
Fix for Issue #496 - Safe Name Generation for FromExistingCode Projects #1181
Conversation
…e tag to match target replacement filename.
…letion of appropriate directory folders for each respective project created.
Hi @AustinHull, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
@@ -91,6 +94,7 @@ Global | |||
{240A6D2B-1982-4BC5-B1EA-C6CB0492329D}.Release|x86.Build.0 = Release|x86 | |||
{D092D54E-FF29-4D32-9AEE-4EF704C92F67}.Debug|x86.ActiveCfg = Debug|x86 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the changes in this file. I don't think any updates are needed based on the rest of the changes.
Thank you for this contribution. I think it really makes the overall experiance more consistent, even with the small known issue. Just a few minor comments to take a look at and we can merge this in. |
…ntation within ImportWizard.cs.
Thank you very much for the feedback! I'm very glad to have been of help! I believe my latest commit should take care of the loose ends you've pointed out. Let me know if I missed anything, or if I should make an additional change. Thank you again! |
Directory.Delete(replacementsDictionary["$destinationdirectory$"]); | ||
Directory.Delete(replacementsDictionary["$solutiondirectory$"]); | ||
// Regardless of whether or not an exception is thrown, created | ||
// directories shall remain. | ||
} catch { | ||
// If it fails (doesn't exist/contains files/read-only), let the directory stay. | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just delete this whole block along with any comments it contains since the body of the try
is now empty.
Understood. I wasn't sure if removing it would cause unpredictable behavior with regards to exception handling, so I was hesitant to do so at first, but I've removed it now. Thanks again! |
Thanks for the contribution! The change should be in our next devbuild. We'll take this into the next major release instead of 1.2, since it does change the behavior |
closes #496
The bug in question would occur whenever the user would create a new project of the "FromExistingCode" template. Each attempt at creating a new project would yield a suggested name suffixed by the number 1, oddly causing a series of identically-numbered projects which would then be further suffixed with a separate, sequential number. I have determined that the primary culprit of this bug can likely be pinned as the project type's lack of project directory generation, as opposed to other projects which generate names appropriately.
Example of bug, as provided in Issue #496 by @hoanhtien :
Being little experienced with this software's architecture, I started by comparing both the *.vstemplate files and various Wizard.cs files between the FromExistingCode project type and others, such as the NodejsWebApp project type. In the FromExistingCode.vstemplate file, I noted that the CreateNewFolder tag was set as false, whereas other project templates had this attribute marked as true, so I changed said flag appropriately. I also noticed that within the ImportWizard.cs file, code was present which would delete any generated project directories. I'm not sure why this code was present, or what purpose It served, but commenting it out and making slight alterations to the directory-setting code (not too far below the aforementioned code) allows for the generation of a unique project directory for each project, and if the "Create Directory" box is checked in the Project Wizard, an additional directory will be created within the general project directory. Regardless of which of these options is chosen, the generation of the general project directory appears to solve the name generation issue.
New outputs, resulting from fixes - Below example is of "Create Directory" project:
This example of non-"Create Directory" project (note the unique project directories, while the *.njsproj files remain in Visual Studio's Projects directory:
Known Issues:
I've noticed, during walkthrough of the FromExistingCode Setup Wizard, that if the user clicks "Cancel" before completing the setup process, the unique directories will still have been created, despite the *.njsproj file itself not being created. Subsequent project creations will still be given a unique-generated name, but the number will be incremented as if the previous project WAS created. This, of course, can be rectified by simply deleting the empty/unused project directory.
Thank you so much for your time and for the opportunity to contribute. It's all greatly appreciated!