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 DiagnosticScenarios #1378

Merged
merged 9 commits into from
Sep 6, 2019
Merged

Conversation

sdmaclea
Copy link
Contributor

Summary

Add sample debug target for use in C# Guide diagnostics scenarios.

@sdmaclea sdmaclea self-assigned this Aug 29, 2019
Add sample debug target for use in C# Guide diagnostics scenarios.
Add whitespace arounf operators
Prefer implicit types when right side type is immediately obvious
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thanks, @sdmaclea. I've left one comment, and @Youssef1313 left several. We shuld also add the sample to the Samples Browser. I've left a second comment with the metadata needed to do that.

@sdmaclea
Copy link
Contributor Author

Thanks @Maoni0

I have applied all feedback.

I created the related docs PR. #dotnet/docs#14110. It's WIP.

@rpetrusha
Copy link

Thanks for making these changes, @sdmaclea. This looks good, although the sample isn't building on the browser review site. Let's leave this open until Tuesday, when I should be able to find out why the sample isn't building.

@sdmaclea
Copy link
Contributor Author

I tried to build it locally. I found one of the var changes was a syntax error. It should build now. Tuesday is fine.

@Youssef1313
Copy link
Member

Youssef1313 commented Aug 31, 2019

@sdmaclea, Not sure what causes the usage of var to cause a syntax error. for me, seems that both should build and compile successfully.

image

but the usage of var follows the coding conventions specified by both, this article and corefx coding style.

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Sep 3, 2019

@Youssef1313

var is recommended in the article you cite for local variables. The failing case was for class member variables, where they are not recommended and cause the compiler to issue an error (at least by default).

Also the corefx guidance is specifically describes when to not use var.

We only use var when it's obvious what the variable type is (e.g. var stream = new FileStream(...) not var stream = OpenStandardInput()).

@Youssef1313
Copy link
Member

@sdmaclea, Oh. Missed the point that it's not a local variable here. Sorry for that.

@rpetrusha
Copy link

I'll merge this sample, @sdmaclea, and then we'll see whether it builds successfully.

@rpetrusha rpetrusha merged commit 821b85c into dotnet:master Sep 6, 2019
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