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

Handle output filename when targetting .Net Framework #28

Merged
merged 7 commits into from
Jul 24, 2018
Merged

Handle output filename when targetting .Net Framework #28

merged 7 commits into from
Jul 24, 2018

Conversation

mrjpeterj
Copy link
Contributor

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.

Copy link
Member

@ChrisMaddock ChrisMaddock left a 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!

// 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;
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

@ChrisMaddock ChrisMaddock Jul 20, 2018

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!

// if TargetFramework is netcore
string outputType = "dll";

if (!targetFramework.StartsWith("netcore"))
Copy link
Member

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!)

Copy link
Contributor Author

@mrjpeterj mrjpeterj Jul 20, 2018

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

XmlNode outputTypeNode = _doc.SelectSingleNode("Project/PropertyGroup/OutputType");
if (outputTypeNode != null)
{
outputType = outputTypeNode.InnerText;
Copy link
Member

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!)

Copy link
Contributor Author

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

Copy link

@jnm2 jnm2 Jul 20, 2018

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).

Copy link
Member

@rprouse rprouse left a 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 😄

@mrjpeterj
Copy link
Contributor Author

Looks all like whitespace changes.
The current version in master has a couple of line endings of \r\n
My commits ended up fully moving to \r\n which obvs produced a huge conflict, so I converted my file to \n only, which therefore results in those few \r\n lines being modified.

@ChrisMaddock
Copy link
Member

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!

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.

4 participants