-
Notifications
You must be signed in to change notification settings - Fork 9
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
Handle output filename when targetting .Net Framework #28
Conversation
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.
Hey @mrjpeterj - thanks for getting round to this so quickly! I've left comments on a couple of pieces I'm unsure about whether this change will have adverse effects - would be great if you could take a look and let me know what you think.
Secondly - it would be good to add a couple of tests for the format you're fixing, to avoid regression in the future. From memory, there's a fairly standard pattern in the test suite of this repository of proj files which are all tested - let us know if you need any guidance on what to put where!
Process on your side was perfect - thanks! 😄 On our side, this will also need reviewing by a second @nunit/framework-and-engine-team member before merging - so you may wish to wait for that feedback before going back to the code - someone else might identify something I've missed!
src/extension/VSProject.cs
Outdated
// When targetting standard .Net framework, the default is still dll, | ||
// however, OutputType is now honoured. | ||
// Also if Sdk = 'Microsoft.NET.Sdk.Web' then OutputType default is exe | ||
string sdk = root.Attributes["Sdk"].Value; |
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.
The docs below suggest the sdk attribute might not always be present - if it was missing, I think this line would throw a NullReferenceException, wouldn't it? Can we guard against that?
https://docs.microsoft.com/en-us/dotnet/core/tools/csproj#sdk-attribute
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.
This whole block of code is conditional on the sdk attribute existing, because otherwise it isn't a .Net Core project -
You need to have the Sdk attribute set to one of those IDs on the <Project> element in order to use the .NET Core tools and build your code.
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.
You're right, it's safe checked earlier in the method, isn't it. Thanks!
src/extension/VSProject.cs
Outdated
// if TargetFramework is netcore | ||
string outputType = "dll"; | ||
|
||
if (!targetFramework.StartsWith("netcore")) |
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.
This may be a non-issue, but what about netstandard projects? Does this fix need to be specific to net20-net472 target frameworks, rather than "anything except netcore"? (I'm not sure on the answer here - but don't want this change to affect other functionality!)
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.
Yes. I wasn't sure on what actual test to do here.
Really I'd want to test for 'net[0-9]' but I think I was being lazy.
I'll improve it in some way
src/extension/VSProject.cs
Outdated
XmlNode outputTypeNode = _doc.SelectSingleNode("Project/PropertyGroup/OutputType"); | ||
if (outputTypeNode != null) | ||
{ | ||
outputType = outputTypeNode.InnerText; |
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.
This is correct for exe's, but aren't dll's shown as <OutputType>Library</OutputType>
?
Would we be safer to do if (outputTypeNode == "Exe") outputType = "exe" else outputType = "dll"
?
(Or at least the above idea, written in a nicer fashion that I've managed on the train!)
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.
I'll look into that.
It might well be an extra case that if you change the output type to dll that it adds this in.
Most of the projects that I made to test against were just all of the different flavours that I could find to create.
I didn't get as far as looking what you might be able to produce in your project file if you messed about with it a lot
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.
The three output types I'm familiar with are Library
, Exe
and WinExe
(to suppress showing the console).
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.
This looks good, but before I approve, what are all the unrelated changes? It looks like the line endings got changed on a bunch of lines? Were they just /n before and got updated to /r/n? Hard to see in a GitHub diff 😄
Looks all like whitespace changes. |
Changes look great to me - thanks for adding all those test files, that makes me feel much more comfortable now all that knowledge is pinned down! 😄 Our whitespace is inconsistent unfortunately, but I don't see anything untoward in the changes there. I'll merge this now - thanks for the contribution! 👍 We'll do another release soon - hopefully in the next week or so - and get this out in the wild for everyone to use! |
Proposed fix for #27
Apologies if I haven't followed the process properly.
This is my first time at contributing to anything in this way.