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 descriptions of source compatible and binary compatible changes #7803

Merged
merged 5 commits into from
Sep 25, 2018

Conversation

BillWagner
Copy link
Member

@BillWagner BillWagner commented Sep 20, 2018

Fixes #3717

Changes in this PR:

Add article that defines source breaking change and binary breaking change when using new language features in a library. This is ready for a thorough review to make sure I've expressed the concepts in the above issue correctly, and in a manner that is useful.

Add note about compatibility on some feature descriptions in what's new articles. This is the second pass of the review, if the concepts are described correctly. I took the following strategy:

  1. Only add a note on features that could be part of the public API surface for a type. Other feature descriptions have not been updated. My choice is somewhat subjective. For example, I did not say anything on the feature for new syntax for numeric literals. A numeric literal could be used for a default value of an argument, but that seemed a stretch, and not useful information to add. This PR adds notes when a new feature appears in a member's signature (except for expression bodied members where there have been customer questions).
  2. I tried to be concise in calling this out, but still accurate. Question for reviewers: Does that make any of the notes misleading?

The discussion on the issue referenced has examples and details that will be helpful for reviewers.

/cc @jcouv @jaredpar @JonSkeet (who wrote the comment for the original issue)

internal review link - new article
update - what's new in 6
update - what's new in 7
update - what's new in 7.2
update - what's new in 7.3
What's new in 7.1 was not updated.

Changing "public struct" to "public readonly struct" is binary compatible.
Copy link
Contributor

@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.

This is a really valuable topic, @BillWagner. (I've never thought about compatibility across compiler versions, only framework versions.) I've left a number of questions, comments, and suggestions.

docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/csharp-7-2.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/csharp-7-3.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/csharp-7.md Outdated Show resolved Hide resolved
@BillWagner
Copy link
Member Author

Thanks @rpetrusha and @pkulikov

I've udpated with changes that address all the comments.

@BillWagner BillWagner changed the title [WIP] Add descriptions of source compatible and binary compatible changes Add descriptions of source compatible and binary compatible changes Sep 21, 2018
Copy link
Contributor

@pkulikov pkulikov left a comment

Choose a reason for hiding this comment

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

@BillWagner Thank you for addressing comments. I like a new version. Found a couple of nits to consider/fix.

docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
@BillWagner BillWagner force-pushed the discuss-version-issues branch from 4d2b970 to c33c61e Compare September 23, 2018 18:30
Copy link
Contributor

@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.

This is a really good article, @BillWagner. I've left a few more suggested changes, but you can merge when you're ready.

docs/csharp/whats-new/csharp-7-2.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
docs/csharp/whats-new/version-update-considerations.md Outdated Show resolved Hide resolved
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM modulo typo. Thanks!

@BillWagner BillWagner merged commit 8ba848d into dotnet:master Sep 25, 2018
@BillWagner BillWagner deleted the discuss-version-issues branch September 25, 2018 03:18
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.

4 participants