Skip to content

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Jun 30, 2020

@hvitved hvitved added C# C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR labels Jun 30, 2020
@hvitved hvitved marked this pull request as ready for review June 30, 2020 14:25
@hvitved hvitved requested review from a team as code owners June 30, 2020 14:25
@hvitved hvitved requested a review from rdmarsh2 July 1, 2020 07:58
@hvitved hvitved force-pushed the csharp/autobuilder-refactor branch from 671c927 to 398a95c Compare July 1, 2020 18:07
Copy link
Contributor

@rdmarsh2 rdmarsh2 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll give @calumgrant a chance to give his feedback as well.

Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

Overall LGTM - a small suggestion.

{
private readonly Func<Autobuilder, Func<IDictionary<string, string>?, BuildScript>, BuildScript> withDotNet;
Copy link
Contributor

Choose a reason for hiding this comment

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

Func<Autobuilder, Func<IDictionary<string, string>?, BuildScript>, BuildScript> could be turned into a delegate as it's written out 4 times.

Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

Agreed to make changes in follow-up. Thanks Tom!

@hvitved hvitved merged commit d01904d into github:master Jul 2, 2020
@hvitved hvitved deleted the csharp/autobuilder-refactor branch July 2, 2020 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants