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 messages to hook result. #21 #23

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Add messages to hook result. #21 #23

merged 1 commit into from
Jul 24, 2018

Conversation

BugDiver
Copy link
Member

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 23, 2018
@ghost ghost assigned BugDiver Jul 23, 2018
@ghost ghost added the in progress label Jul 23, 2018
@@ -44,7 +45,8 @@ public override Message Process(Message request)
if (debuggingEnv != null && debuggingEnv.ToLower().Equals("true"))
{
// if the runner is launched in DEBUG mode, let the debugger attach.
System.Console.WriteLine("Runner Ready for Debugging at Process ID " + System.Diagnostics.Process.GetCurrentProcess().Id);
Console.WriteLine("Runner Ready for Debugging at Process ID " +
System.Diagnostics.Process.GetCurrentProcess().Id);
Copy link
Member

Choose a reason for hiding this comment

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

can be Process.GetCurrentProcess since System.Diagnostics is already in using.

Copy link
Member Author

Choose a reason for hiding this comment

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

That creates name collision with the method name (Process).

@@ -64,38 +64,6 @@ public void Setup()
};
}

//[BeforeScenario("Foo")]
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't commented code :) - this bit of code block was to indicate what is the logic being tested. It was to ease the data setup. Perhaps you can add a line describing such, but i prefer we leave this in place.

@@ -56,42 +56,6 @@ IHookMethod Create(string name, int aggregation = 0, params string[] tags)
};
}

//[AfterScenario("Foo")]
Copy link
Member

Choose a reason for hiding this comment

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

This bit of code block was to indicate what is the logic being tested. It was to ease the data setup. Perhaps you can add a line describing such, but i prefer we leave this in place.

@@ -55,43 +55,6 @@ IHookMethod Create(string name, int aggregation = 0, params string[] tags)
};
}


//[AfterScenario("Foo")]
Copy link
Member

Choose a reason for hiding this comment

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

This bit of code block was to indicate what is the logic being tested. It was to ease the data setup. Perhaps you can add a line describing such, but i prefer we leave this in place.

@BugDiver BugDiver force-pushed the custom_messages branch 2 times, most recently from df6f8d7 to e0de8dc Compare July 24, 2018 10:16
@sriv sriv merged commit 12969f3 into master Jul 24, 2018
@ghost ghost removed the in progress label Jul 24, 2018
@sriv sriv deleted the custom_messages branch July 24, 2018 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants