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.

Sign up for GitHub

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

Jump to bottom

[StyleCop] Fix all the warnings on ApplicationInsights.Tests #1507

Merged
merged 1 commit into from
Mar 25, 2019
Merged

[StyleCop] Fix all the warnings on ApplicationInsights.Tests #1507

merged 1 commit into from
Mar 25, 2019

Conversation

ceciliaavila
Copy link
Collaborator

  • Rename variable to avoid Hungarian notation
  • Fix spacing
  • Fix parameters

@ceciliaavila ceciliaavila mentioned this pull request Mar 18, 2019
Closed
@ParadoxARG ParadoxARG force-pushed the fix/stylecop/ApplicationInsights.Tests branch from c85f7be to 8ddc5b6 Compare March 19, 2019 20:33
@coveralls
Copy link
Collaborator

coveralls commented Mar 19, 2019 •
edited

Pull Request Test Coverage Report for Build 52003

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 75.588%
Totals Coverage Status
Change from base Build 51937: 0.0%
Covered Lines: 4304
Relevant Lines: 5694
💛 - Coveralls

@ParadoxARG ParadoxARG marked this pull request as ready for review March 19, 2019 20:55
munozemilio
munozemilio suggested changes Mar 21, 2019
View reviewed changes
Copy link
Member

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

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

There is an empty string added. Was this intended?
It didn't exist before.

Thank you

@ParadoxARG ParadoxARG force-pushed the fix/stylecop/ApplicationInsights.Tests branch from 8ddc5b6 to ef0d4e2 Compare March 21, 2019 14:13
munozemilio
munozemilio suggested changes Mar 21, 2019
View reviewed changes
Copy link
Member

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

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

On a second look I noticed there is a new method introduced that can reduce code duplication.
I provided a few comments.

Thanks!

@ParadoxARG
Copy link
Contributor

ParadoxARG commented Mar 21, 2019 •
edited

On a second look I noticed there is a new method introduced that can reduce code duplication.
I provided a few comments.

Thanks!

Hi @munozemilio! I think that's a great idea. If you think it's fine we will be creating a new PR with just
those changes to dissociate them from the stylecop ones.

@munozemilio
Copy link
Member

Hello @ParadoxARG, absolutely agree with you. Those refactors should go in a different PR. I will then request a change on the first waterfall dialog to keep it consistent and remove the NewWaterFallSteps method, and just introduce the StyleCop fixes.

Thanks!

munozemilio
munozemilio reviewed Mar 21, 2019
View reviewed changes
tests/Microsoft.Bot.Builder.ApplicationInsights.Tests/TelemetryWaterfallTests.cs Outdated
async (step, cancellationToken) => { await step.Context.SendActivityAsync("step3"); return Dialog.EndOfTurn; },
}));
dialogs.Add(new WaterfallDialog(
"test", NewWaterfall()));
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep it consistent with the StyleCop changes and not introduce any refactors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed! Moving it to a new PR

munozemilio
munozemilio reviewed Mar 21, 2019
View reviewed changes
tests/Microsoft.Bot.Builder.ApplicationInsights.Tests/TelemetryWaterfallTests.cs Outdated
// Event contains "StepName"
// Event naming on lambda's is "StepXofY"
Assert.IsTrue(saved_properties["WaterfallCancel_4"]["StepName"] == "Step3of3");
Assert.IsTrue(waterfallDialog.CancelDialogCalled);
Assert.IsFalse(waterfallDialog.EndDialogCalled);
}

private static WaterfallStep[] NewWaterfall()
Copy link
Member

Choose a reason for hiding this comment

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

Since the intention of the PR is only to fix StyleCop issues, can we push this in a different PR to keep it consistent?

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed! Moving it to a new PR

@ParadoxARG ParadoxARG force-pushed the fix/stylecop/ApplicationInsights.Tests branch from ef0d4e2 to bf17465 Compare March 22, 2019 14:26
@ParadoxARG ParadoxARG force-pushed the fix/stylecop/ApplicationInsights.Tests branch from bf17465 to fbafcda Compare March 22, 2019 15:48
@ParadoxARG
Copy link
Contributor

Rebased to master and applied feedback

This was referenced Mar 22, 2019
Closed
Merged
@munozemilio munozemilio merged commit a7f1804 into master Mar 25, 2019
@munozemilio munozemilio deleted the fix/stylecop/ApplicationInsights.Tests branch March 25, 2019 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ParadoxARG ParadoxARG ParadoxARG left review comments

@munozemilio munozemilio munozemilio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ceciliaavila @coveralls @ParadoxARG @munozemilio