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

Add ability to override defaults #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kingerja
Copy link

I didn't fully test the backcompat scenario but for basic usages it appears to work fine.

The intention of my change was to give the ability to give the user a change to customize with out having to duplicate your targets and maintain a separate copy of them. I also implemented it in a way that I believe will provide for back compat for those who are still dependent on your task.

Anyway feel free to reject this change I just thought I would contribute it back.

@@ -71,6 +71,10 @@
<SubType>Designer</SubType>
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="ILRepack.Lib.MSBuild.Task.props">
<SubType>Designer</SubType>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can always delete SubType Designer

@@ -71,6 +71,10 @@
<SubType>Designer</SubType>
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</None>
<None Include="ILRepack.Lib.MSBuild.Task.props">
<SubType>Designer</SubType>
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
Copy link
Contributor

@KirillOsenkov KirillOsenkov Dec 24, 2023

Choose a reason for hiding this comment

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

Always breaks incrementality, it's better to use PreserveNewest

@@ -1,5 +1,7 @@
<?xml version="1.0" encoding="utf-8" ?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for an empty PropertyGroup here

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