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

Remove duplicate classes topic from C# guide #4889

Merged
merged 8 commits into from
Apr 9, 2018
Merged

Remove duplicate classes topic from C# guide #4889

merged 8 commits into from
Apr 9, 2018

Conversation

pkulikov
Copy link
Contributor

@pkulikov pkulikov commented Apr 5, 2018

Combined C# Concepts classes article into the Classes (C# Programming guide) article.

Additionally:

  • Inlined small code pieces as it makes a source Markdown file more readable.
  • Minor editions.

Fixes #964

In tandem with PR dotnet/samples#8

@pkulikov pkulikov changed the title Remove classes duplication Remove duplicate classes topic from C# guide Apr 5, 2018
@pkulikov pkulikov changed the title Remove duplicate classes topic from C# guide WIP Remove duplicate classes topic from C# guide Apr 6, 2018
@pkulikov
Copy link
Contributor Author

pkulikov commented Apr 6, 2018

@BillWagner I put WIP as I also want to move the example snippet to the samples repo.

@mairaw
Copy link
Contributor

mairaw commented Apr 6, 2018

@pkulikov so we have a current work item (#1255) that we haven't been able to finish yet (it was supposed to be automated) but I manually change some PRs when I see they're touching snippets in these codesnippet folders.

The first time we migrated these topics, they erroneously broken down each sample into these tiny little snippets. But we do have the snippets in the samples repo already and you can see how the topic should have looked like if it got migrated correctly the first time here:
https://raw.githubusercontent.com/dotnet/docs/migration_csharpvb_reexport/docs/csharp/programming-guide/classes-and-structs/classes.md

So the original snippet for this probably lives here:
https://github.com/dotnet/samples/blob/master/snippets/csharp/VS_Snippets_VBCSharp/csProgGuideObjects/CS/Objects.cs

But you can probably change the samples location to ~/samples now that they don't live in this repo anymore. Something to test.

@pkulikov
Copy link
Contributor Author

pkulikov commented Apr 6, 2018

@mairaw interesting that that file with a lot of snippets doesn't contain snippets for the topic in interest; as if those were cut out of the file.

I've made the PR in the samples repo to move the snippet to programming guide snippets here:
https://github.com/dotnet/samples/tree/master/snippets/csharp/programming-guide/classes-and-structs

I assume the old way to link snippets from the Markdown files continues to work even when snippets are in the different repo.

This PR is ready for review.

@pkulikov pkulikov changed the title WIP Remove duplicate classes topic from C# guide Remove duplicate classes topic from C# guide Apr 6, 2018
@pkulikov
Copy link
Contributor Author

pkulikov commented Apr 6, 2018

@mairaw I've just realized that, of course, build fails - I should wait for PR dotnet/samples#8 in the samples repo to be merged in the repo. However, that would break current master here. How do I go from here? (Option: update PR in the samples repo not to break master here.)

@mairaw
Copy link
Contributor

mairaw commented Apr 6, 2018

It's something we also need to understand better ourselves. It's been in my to-do list to talk to the engineering team about this workflow. When I catch a break, I'll take a look at your changes here and there. Exciting times! 😄

@pkulikov
Copy link
Contributor Author

pkulikov commented Apr 6, 2018

We need a notion of tandem PRs :)

@pkulikov
Copy link
Contributor Author

pkulikov commented Apr 9, 2018

@BillWagner there was the merge conflict with master; I've resolved it with the latest commit

@pkulikov
Copy link
Contributor Author

pkulikov commented Apr 9, 2018

@BillWagner and we should merge this PR as fast as possible: all other PRs do not build because of this one.

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

@pkulikov Overall, this is a great improvement to combine these two topics. There were a couple sentences that could be misinterpreted. I offered suggestions for other wording. Once that's done, I'll :shipit:

Thanks again.

A *class* is a construct that enables you to create your own custom types by grouping together variables of other types, methods and events. A class is like a blueprint. It defines the data and behavior of a type. If the class is not declared as static, client code can use it by creating *objects* or *instances* which are assigned to a variable. The variable remains in memory until all references to it go out of scope. At that time, the CLR marks it as eligible for garbage collection. If the class is declared as [static](../../../csharp/language-reference/keywords/static.md), then only one copy exists in memory and client code can only access it through the class itself, not an *instance variable*. For more information, see [Static Classes and Static Class Members](../../../csharp/programming-guide/classes-and-structs/static-classes-and-static-class-members.md).

Unlike structs, classes support *inheritance*, a fundamental characteristic of object-oriented programming. For more information, see [Inheritance](../../../csharp/programming-guide/classes-and-structs/inheritance.md).
A *class* is a construct that enables you to create your own custom types by grouping together variables of other types, methods and events. A class is like a blueprint. It defines the data and behavior of a type. If the class is not declared as static, client code can create *instances* of a class or *objects* which are assigned to a variable. The instance of a class remains in memory until all references to it go out of scope. At that time, the CLR marks it as eligible for garbage collection. If the class is declared as [static](../../../csharp/language-reference/keywords/static.md), then only one copy exists in memory and client code can only access it through the class itself, not an *instance variable*. For more information, see [Static Classes and Static Class Members](../../../csharp/programming-guide/classes-and-structs/static-classes-and-static-class-members.md).
Copy link
Member

Choose a reason for hiding this comment

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

There are a few sentences here that could be more clear:

If the class is not declared as static, client code can create instances of a class or objects which are assigned to a variable.

Consider: "If the class is not declared as static, client code can create instances of it. These instances are objects which are assigned to a variable."

If the class is declared as static, then only one copy exists in memory and client code can only access it through the class itself, not an instance variable.

Consider: "If the class is declared as static, you cannot create instances, and client code can only access it through the class itself."

@pkulikov
Copy link
Contributor Author

pkulikov commented Apr 9, 2018

@BillWagner thank you for feedback, I've updated wording as you've suggested.

@mairaw
Copy link
Contributor

mairaw commented Apr 9, 2018

If you approved this @BillWagner, I'll merge it then because all builds are breaking now because the samples change is in but not this one.

@mairaw mairaw merged commit ea49a31 into dotnet:master Apr 9, 2018
@pkulikov pkulikov deleted the remove-classes-duplication branch April 9, 2018 16:34
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.

3 participants