-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
C# Versioning #1197
C# Versioning #1197
Conversation
Open Publishing Build Service: The pull request content has been published and here are some changed files links of this time:
Status: SucceededWithWarning. Message: Warning occurred: docs/csharp/csharp-7.md contains illegal link: #local-expressions. The file docs/csharp/csharp-7.md doesn't contain a bookmark named local-expressions... |
Open Publishing Build Service: The pull request content has been published and here are some changed files links of this time:
Status: SucceededWithWarning. Message: Warning occurred: docs/core/tools/project-json.md contains illegal link: #supressParent. The file docs/core/tools/project-json.md doesn't contain a bookmark named supressParent... |
@tsolarin Sorry for a delay on this. I was traveling. I'll get lots of feedback today. |
@tsolarin This is a good start. I'm going to make conceptual comments, rather than review it line by line. First, we've added a continuous integration service that builds all the code from all the samples. docfx supports an include syntax so you can add the full sample code to the repository. That way, it gets built and we ensure there are no errors. The include syntax enables parts of the sample to be published in the article. See the updates in PR #1209 for the syntax for the includes. The contributing guide contains information on where to put sample code so the CI build will find them. The article as it stands reads more about override and object oriented techniques than about versioning. We need to organize the material differently to make the versioning information stand out. I'd lead with a discussion of semantic versioning including a link to semver.org. That describes guidelines for updating an assembly. I'd discuss the element in app.config and how that enables runtime updates without recompiling. Next, I'd discuss the Finally, I'd only briefly discuss If you have any questions, just message me here. And thank you again for all the continued contributions. |
Added the Work in Progress (WIP) label pending the next draft. |
@BillWagner thanks for the review, I'll take a look at code inclusion you mentioned. I also would like to mention that the file I'm working with has no documentation metadata like the others I've worked with, how do I fix this? I'll start working on the next draft based on your recommendations |
Here's a block you can use:
|
Open Publishing Build Service: The pull request content has been published and here are some changed files links of this time:
Status: SucceededWithWarning. Message: Warning occurred: articles/standard/base-types/extract-day.html contains illegal link: core/api/System.DateTime.html#System.DateTime.Parse(System.String). The file core/api/System.DateTime.html doesn't contain a bookmark named System.DateTime.Parse(System.String), please check the src file docs/standard/base-types/extract-day.md and src linkedTo file api_ref/System.DateTime.yml or the template you applied... |
@BillWagner ready for another go |
❌ Validation status: errors
docs/csharp/versioning.md
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
❌ Validation status: errors
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
@tsolarin I'm flying most of the day today. I'll have comments by the end of the day on Monday. |
@tsolarin Let's go one more content round. You've got much of the facts here, but the story is missing. This one could be hard, because there are really two stories:
The override section should be a sub-set of the second section, and its only purpose should be to explain how method binding works for Does that make sense? |
@BillWagner sure it does make a lot of sense. I was trying to merge building libraries and consuming libraries into one, I guess separating them out into sections will be much better. I'll get to working on another draft. |
/cc @BillWagner ready for another go |
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
@tsolarin I'll try to review this by the end of the week. With the American Thanksgiving, it might be early next week. Is that OK? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is on the right track. I gave it a thorough review, and I think it's getting close. One more cycle, and I think we'll be down to a quick copy edit.
--- | ||
|
||
# Versioning in C# | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a brief introduction here to tell readers what they will learn by reading this article. One or two paragraphs is all it should take.
There are also ways to specify other scenarios like pre-release versions etc. Go [here](http://semver.org/) to get more information on SemVer | ||
and how best to utilize it when applying version information to your .NET library. | ||
|
||
### Application Configuration File |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this section below the Backwards Compatibility section. I also think it needs a sample to demonstrate loading some data that changes the library's behavior.
|
||
As you release new versions of your library, backwards compatibility with previous versions will most likely be one of your major concerns. | ||
A new version of your library is source compatible with a previous version if code that depends on the previous version can, when recompiled, work with the new version. | ||
In contrast, a new version of your library is binary compatible if an application that depended on the old version can, without recompilation, work with the new version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd delete "In contrast". The two definitions read quite well without it.
will have to be updated. | ||
* Method signatures: When updating a method behaviour requires you to change it's signature as well, you should instead create an override so that derived classes calling into that method will still work. | ||
You can always manipulate the old method signature to call into the new method signature so that implementation remains consistent. | ||
* Optional Method Arguments: When you make previously optional method arguments compulsory then all code that does not supply those arguments will need to be updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should mention that changing the default value of an argument can be a breaking change as well.
* Method signatures: When updating a method behaviour requires you to change it's signature as well, you should instead create an override so that derived classes calling into that method will still work. | ||
You can always manipulate the old method signature to call into the new method signature so that implementation remains consistent. | ||
* Optional Method Arguments: When you make previously optional method arguments compulsory then all code that does not supply those arguments will need to be updated. | ||
On the other hand making compulsory arguments optional should have very little effect especially if it doesn't change the method's behaviour. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might read better as a Note. (See the style guide for the note style)
On the other hand making compulsory arguments optional should have very little effect especially if it doesn't change the method's behaviour. | ||
* Obsolete attribute: You can use this attribute in your code to specify classes or class members that are deprecated and likely to be removed in future versions. | ||
This ensures developers utilizing your library are better prepared for breaking changes. More info on the `Obsolete` attribute [here](https://msdn.microsoft.com/en-us/library/22kk2b44(v=vs.90).aspx). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a closing before you go on. Consider this point: the easier you make it for your users to upgrade to the new version of your library, the more likely that they will upgrade sooner.
|
||
### Assembly Binding Redirection | ||
|
||
You can use the `app.config` file to update the version of a library your app uses. By adding what is called a `binding redirect` your |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should style it binding redirect as a definition instead of binding redirect
as a code style.
|
||
You can use the `app.config` file to update the version of a library your app uses. By adding what is called a `binding redirect` your | ||
can use the new library version without having to recompile your app. The following example shows how you would update | ||
your app's `app.config` file to use the `1.0.1` patch version of `ReferencedLibrary` instead of the `1.0.0` version it was originally compiled with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would reference the section above where you talk about how to ensure binary compatibility.
|
||
### new | ||
|
||
You use the `new` modifier to hide inherited members of a base class. This ensures resiliency of derived classes allowing you to update base classes the way you want in your library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second sentence ready very awkward to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the sentence
``` | ||
|
||
The `override` modifier is evaluated at compile time as opposed to at runtime for the `new` modifier, and the compiler will throw an error | ||
if it doesn't find a virtual member to override. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, as well, you need a good conclusion about what you covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a conclusion although I don't think what I wrote gives the article a strong ending. A recommendation will be appreciated
ms.assetid: aa8732d7-5cd0-46e1-994a-78017f20d861 | ||
--- | ||
|
||
# Versioning in C# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be # Versioning in C# #
, otherwise the #
will not be visible.
* `MINOR` is incremented when you add functionality in a backwards-compatible manner | ||
* `PATCH` is incremented when you make backwards-compatible bug fixes | ||
|
||
There are also ways to specify other scenarios like pre-release versions etc. Go [here](http://semver.org/) to get more information on SemVer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Go [here]
is generally not considered a good style for links. Instead, I would link a relevant phrase, like "Semantic versioning" here or "the app.config
file" below.
|
||
* Virtual methods: When you make a virtual method non-virtual in your new version it means that projects that override that method | ||
will have to be updated. | ||
* Method signatures: When updating a method behaviour requires you to change it's signature as well, you should instead create an override so that derived classes calling into that method will still work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: it's → its
|
||
* Virtual methods: When you make a virtual method non-virtual in your new version it means that projects that override that method | ||
will have to be updated. | ||
* Method signatures: When updating a method behaviour requires you to change it's signature as well, you should instead create an override so that derived classes calling into that method will still work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should instead create an override
Did you mean "overload" instead of "override"? And why are you mentioning derived classes, doesn't this apply to any class that calls the method? Maybe I just don't understand what are you trying to say here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did mean overload
thanks for pointing that out. My mention of derived classes was me coming from the angle of inheritance, it definitely makes sense to mention all code that calls into that method. Thanks
* Optional Method Arguments: When you make previously optional method arguments compulsory then all code that does not supply those arguments will need to be updated. | ||
On the other hand making compulsory arguments optional should have very little effect especially if it doesn't change the method's behaviour. | ||
* Obsolete attribute: You can use this attribute in your code to specify classes or class members that are deprecated and likely to be removed in future versions. | ||
This ensures developers utilizing your library are better prepared for breaking changes. More info on the `Obsolete` attribute [here](https://msdn.microsoft.com/en-us/library/22kk2b44(v=vs.90).aspx). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should link to docs.MS instead of MSDN. And I would avoid using [here]
as well. The two changes put together:
* [Obsolete attribute](programming-guide/concepts/attributes/common-attributes.md#Obsolete): You can use this attribute …
|
||
``` | ||
[!code-csharp[Sample usage of the 'new' modifier](/samples/csharp/versioning/new/Program.cs#sample)] | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work. Include syntax should not be encodes in triple backticks. And then the link should be relative:
[!code-csharp[Sample usage of the 'new' modifier](../../samples/csharp/versioning/new/Program.cs#sample)]
You can verify this yourself by building the docs using docfx.
[!code-csharp[Sample usage of the 'new' modifier](/samples/csharp/versioning/new/Program.cs#sample)] | ||
``` | ||
|
||
#### Output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't use a heading for this, since the text after the output does not belong under the heading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made it a bold text
|
||
### override | ||
|
||
The `override` modifier works very similar to the `new` modifier except that it extends the implementation of a base class member rather than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say that it's very similar, the two modifiers are almost opposites.
Derived Method One: Derived Method One | ||
``` | ||
|
||
The `override` modifier is evaluated at compile time as opposed to at runtime for the `new` modifier, and the compiler will throw an error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what is this trying to say. How is new
evaluated at runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my understanding the compiler checks that there's a suitable method to override and replaces that during compile time whereas the CLR determines what method to run (based on the new modifier) when it encounters conflicting member names between the base and derived class. Perhaps, I could've put it a better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLR does not decide anything for calls to methods that are not virtual. It does not even know about the new
keyword, that basically exists just to hide a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Then I guess I've been thinking about it all wrong then. Thanks for setting me in the right direction, I'll go ahead an rephrase that sentence.
@BillWagner @svick Many thanks for your reviews, I've addressed your feedback on my last update. The content is ready for another look over. |
Go [here](https://msdn.microsoft.com/en-us/library/7wd6ex19(v=vs.110).aspx) to get more information on redirecting assembly versions. | ||
> [!NOTE] | ||
> This approach will only work if the new version of `ReferencedLibrary` is binary compatible with your app. | ||
> See the `Backwards Compatibility` section above for changes to look out for when determining compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backtics shouldn't be used here, since it's not code. And since you're referencing a previous section, it would be nice to make it a link:
> See the [Backwards Compatibility](#backwards-compatibility) section above …
|
File | Status | Preview URL | Details |
---|---|---|---|
docs/csharp/versioning.md | View | Details | |
docs/toc.md | ✅Succeeded | View |
docs/csharp/versioning.md
- Line 52: [Warning] Invalid file link:(~/docs/csharp/programming-guide/concepts/attributes/common-attributes.md#Obsolete).
For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
@tsolarin I'm sorry I let this drop. I thought I had responded to the last version, and I see there were updates since my last review. Please accept my apologies. I'm reviewing it now. 😞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting much closer.
I left a few items, but then it's ready.
|
||
Here are some things to consider when trying to maintain backwards compatibility with older versions of your library: | ||
|
||
* Virtual methods: When you make a virtual method non-virtual in your new version it means that projects that override that method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be more prescriptive here. I think it is safe to say that is such a breaking change that this should be strongly discouraged.
### new | ||
|
||
You use the `new` modifier to hide inherited members of a base class. | ||
This makes your derived classes immune to changes made to the inherited members of the base class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd reword this. Instead of "makes your derived classes immune to changes...", I'd say "is one way derived classes can respond to updates in base classes."
|
||
### override | ||
|
||
The `override` modifier is the opposite of the `new` modifier, it extends the implementation of a base class member rather than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say that override is the opposite of new. I'd just say:
"The override
modifier means a derived implementation extends the implementation of a base class member..."
### override | ||
|
||
The `override` modifier is the opposite of the `new` modifier, it extends the implementation of a base class member rather than | ||
hides it, also the base class member needs to have the `virtual` modifier applied to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd split this into two sentences. Starting the second with "The base class member needs..."
keywords: .NET, .NET Core, C# | ||
author: tsolarin | ||
manager: wpickett | ||
ms.date: 2016/12/03 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be mm/dd/yyyy format or the published site shows the wrong date. (I know I gave you this format, but it's a bug we've since discovered.)
No worries @BillWagner, I've also been quite busy as well. I've made the changes you requested |
Thanks for all the work on this @tsolarin We really appreciate it. This looks great. I'll now. |
Thanks to you too @BillWagner. I'm glad I can be of help |
@BillWagner there seems to be a lot of work to do and I'm quite confused what to tackle next. Can you make a suggestion based on set priorities and milestones? |
@tsolarin I'd look at the current month's milestone and look for any issues with the "Jump In' tag. If none of those are interesting, look for any with the P0 or P1 tag that are unassigned. |
This PR adds content for C# Versioning.
Changes include:
Fixes #56
/cc @BillWagner @stevehoag