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

GH1734: Add GitLink 3 aliases #1735

Merged
merged 1 commit into from
Aug 31, 2017
Merged

GH1734: Add GitLink 3 aliases #1735

merged 1 commit into from
Aug 31, 2017

Conversation

Cheesebaron
Copy link
Contributor

This Pull Request relates to #1734 and updates the GitLink tool to reflect the major restructure that was done in GitLink 3.0.0.

Instead of taking a solution directory, the new GitLink tool takes singular pdb files and patches them. I've added a helper alias to allow passing a FilePathCollection to allow globbing.

@dnfclas
Copy link

dnfclas commented Aug 7, 2017

@Cheesebaron,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

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

Use IEnumerable<FilePath> instead of FilePathCollection

@patriksvensson
Copy link
Member

This will be a breaking change for anyone not using GitLink 3.0, right? Any possibility to introduce a GitLink3-runner or similar?

@Cheesebaron
Copy link
Contributor Author

@patriksvensson correct. This could be made as a different runner and alias. I have no issue with doing that instead.

@devlead will fix that.

@devlead
Copy link
Member

devlead commented Aug 7, 2017

Agree with @patriksvensson, could you keep old aliases and add new 3 editions

@Cheesebaron
Copy link
Contributor Author

Should the AliasCategory still be GitLink or a separate GitLink3 one instead?

@devlead
Copy link
Member

devlead commented Aug 7, 2017

smartselectimage_2017-08-07-20-25-22 do seperate like NUnit

@Cheesebaron
Copy link
Contributor Author

Done. Old Gitlink stuff is as is. I've added version pinning in the XML docs for the v2 stuff.

I've renamed all the new GitLink stuff to GitLink3 and made a category called GitLink v3, similar to NUnit.

Copy link
Member

@bjorkstromm bjorkstromm left a comment

Choose a reason for hiding this comment

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

@Cheesebaron builds are failing. Could you try running build scripts locally before pushing changes? .\build.ps1 on Windows or .\build.sh on Linux/OSX.

Tests seems to be hanging, I can see that e.g. TeamCity build times out after 4h running!

/// <para>
/// In order to use the commands for this alias, include the following in your build.cake file to download and
/// install from NuGet.org, or specify the ToolPath within the <see cref="GitLink3Settings" /> class:
/// <code>
Copy link
Member

Choose a reason for hiding this comment

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

Malformed Xml. Remove the extra <para> above.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@Cheesebaron Cheesebaron Aug 8, 2017

Choose a reason for hiding this comment

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

@mholo65 sooooo I just copied what was in: https://github.com/cake-build/cake/blob/develop/src/Cake.Common/Tools/GitLink/GitLinkAliases.cs#L13 it is the same... except I added version 3 after the closing see tag.

Copy link
Member

Choose a reason for hiding this comment

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

I see, it seems to be complaining about version

Tools\GitLink\GitLinkAliases.cs(18,38): error CS1570: XML comment has badly formed XML -- 'version'

On second glimpse, to me that XML looks fine.

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've fixed it. The ampersand needed escaping

@Cheesebaron
Copy link
Contributor Author

@devlead & @mholo65 anything else you guys want me to change? I think I have addressed both the issues you pointed out. I've rebased the PR so many times now, I can rebase it again when you think it is ready to merge.

@gep13 gep13 changed the title Update GitLink tool to use GitLink 3.0.0 GH1734: Update GitLink tool to use GitLink 3.0.0 Aug 8, 2017
@bjorkstromm bjorkstromm changed the title GH1734: Update GitLink tool to use GitLink 3.0.0 GH1734: Add GitLink 3 aliases Aug 9, 2017
@devlead devlead dismissed bjorkstromm’s stale review August 31, 2017 05:57

Feedback addressed.

Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

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

LGTM

@devlead devlead merged commit 87e6139 into cake-build:develop Aug 31, 2017
@devlead
Copy link
Member

devlead commented Aug 31, 2017

@Cheesebaron your changes have been merged, thanks for your contribution 👍

@Cheesebaron Cheesebaron deleted the feature/update-gitlink branch August 31, 2017 14:53
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.

5 participants