Add documentation guidelines for adding new APIs #12036

weshaggard · 2016-09-24T06:06:43Z

@ericstj @stephentoub @joperezr can you guys please review to see if I got all the details?

@danmosemsft @karelz could you guys please dogfood these instructions to see if they help enable you to accomplish the goal?

davidsh · 2016-09-24T19:24:51Z

cc: @CIPop

danmoseley · 2016-09-25T02:03:16Z

cc @zhenlan

danmoseley

Thanks for writing this up. It helps a lot. I"ll take another look after you've gone through this feedback.

danmoseley · 2016-09-25T00:58:34Z

Documentation/coding-guidelines/adding-api-guidelines.md

+`<latest>` is the version currently under development.
+
+- If the library is [part of netstandard](#isnetstandard)
+  - Your target framework should be `netstandard<latest>`


Let's just say what latest is throughout. When latest changes, we can update the document. In general, I suggest couching this document specifically for adding API to .NET Core 1.2 and almost surely part of net standard 2.0.

I was tempted to do that but I really didn't like this document getting outdated everywhere. I will compromise and add the current latest version where I define latest.

danmoseley · 2016-09-25T01:02:57Z

Documentation/coding-guidelines/adding-api-guidelines.md

+
+##Determining versions and targets
+
+1. [Determine what library](#determine-what-library) the API goes into.


This is the design time (reference) library right? In our tree, typically the implementation library has the same name, but not always. Does this document assume they're the same?

All reference assemblies have a matching implementation assembly even if it is just a pure facade. I don't think there is really any assumption in this doc about them needing to be the same, but perhaps I'm missing something.

danmoseley · 2016-09-25T01:04:51Z

Documentation/coding-guidelines/adding-api-guidelines.md

+  - If package dependencies are changed then your target framework should be the minimum target framework that supports all your package dependencies.
+  - If your package depends directly on runtime changes or library changes that ship with the runtime
+    (i.e. System.Private.CoreLib) then your target framework should be `netstandard<latest>` (hint: usually indicated by needing the latest Microsoft.TargetingPack.Private.* package)
+  - If your package depends on changes in a native shim package then your target framework should be


Can you link to example(s) of a native shim package? I don't believe I've encountered one.

danmoseley · 2016-09-25T01:07:01Z

Documentation/coding-guidelines/adding-api-guidelines.md

+  - Your target framework should be `netstandard<latest>`
+  - If it is a new API only available on .NET Core then it will be added to `netcoreapp<latest>`
+- If the library is not part of netstandard
+  - If package dependencies are changed then your target framework should be the minimum target framework that supports all your package dependencies.


Given a package version, how do I determine which is that minimum TFM? Did you mention looking on myget/nuget? Perhaps link to an example.

I don't think it makes sense for folks to do it that way. More often than not you are adding API or impl and you need to dial up the netstandard version to get the necessary API from one of your dependencies.

Yes it should be keyed off your list of dependencies you had to bump.

danmoseley · 2016-09-25T01:10:47Z

Documentation/coding-guidelines/adding-api-guidelines.md

+##Making the changes in repo
+
+**If changing the library version**
+  - Update the `AssemblyVersion` property in `<Library>\dir.props` (ex: [System.Runtime\dir.props](https://github.com/dotnet/corefx/blob/master/src/System.Runtime/dir.props#L4)). If `AssemblyVersion` is


What should I change the assembly version to? Do I just increase the x.minor.x.x version? In corefx\src I see the following assembly versions. Is there a pattern? Were they all the same when we shipped 1.0?
4.0.0.0
4.0.1.0
4.0.10.0
4.0.12.0
4.0.2.0
4.1.0.0
4.1.1.0
4.1.2.0

What should I change the assembly version to? Do I just increase the x.minor.x.x version?

Increase minor and zero the following portions. x.minor.0.0.

The pattern is that minor version changes when API is added. 3rd portion changes when we ship a new assembly without any API additions (bugfixes). Devs don't need to change the third portion, we'll do that for all libraries when we branch for new releases.

Prior to adopting this pattern there was a pattern that didn't encode bugfixes but only incremented the 3rd portion by 10 with API changes. That's where the .10 and .12 are from.

danmoseley · 2016-09-25T02:13:32Z

Documentation/coding-guidelines/adding-api-guidelines.md

+**Update tests**
+  - If changing target framework
+    - Update `NugetTargetMoniker` in `<Library>\tests\<Library>.csproj` (ex: [tests\System.Runtime.csproj](https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System.Runtime.Tests.csproj#L13))
+    - Update framework section in `<Library>\tests\project.json` (ex: [System.Runtime\tests\project.json](https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/project.json#L30))


(General) whereever you use the word Update can you please replace with how it's being updated.

Example: instead of "Update framework section" use "Append the new TFM to the frameworks section"

danmoseley · 2016-09-25T02:16:35Z

Documentation/coding-guidelines/adding-api-guidelines.md

+      - Dependencies on new native shim changes
+    - .NET Native Shared Library
+
+###Potential clean-up work that can be done


Need a pass through the project.json to clean them up. Not just imports but also to remove old frameworks ("net46", "netcore50".. ) as you advised me to do for my recent change.

Also a pass through the .pkgproj to remove old <SupportedFramework> entries such as net45 and wp8.

danmoseley · 2016-09-25T02:18:33Z

Documentation/coding-guidelines/adding-api-guidelines.md

+ - If changing the target framework
+    - Update SupportedFrameworks metadata on the ref ProjectReference to declare the set of
+      concrete of platforms you expect your library to support. (see [Specific platform mappings](https://github.com/dotnet/corefx/blob/master/Documentation/architecture/net-platform-standard.md#nuget)). Generally
+      will be a combination of netcoreapp1.x, netfx46x and/or uap10.x.


and $(AllXamarinFrameworks) ?

danmoseley · 2016-09-25T02:22:23Z

Documentation/coding-guidelines/adding-api-guidelines.md

+
+**Update tests**
+  - If changing target framework
+    - Update `NugetTargetMoniker` in `<Library>\tests\<Library>.csproj` (ex: [tests\System.Runtime.csproj](https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System.Runtime.Tests.csproj#L13))


and add the'$(TargetGroup)' == '' condition if missing?

danmoseley · 2016-09-25T02:25:05Z

Documentation/coding-guidelines/adding-api-guidelines.md

+
+_**What is the difference between being part of netstandard and building against netstandard?**_
+
+Things that are part of netstandard can only change when we release a new version of a platform


Example of each would help.

danmoseley · 2016-09-25T07:51:28Z

Documentation/coding-guidelines/adding-api-guidelines.md

+
+**Update tests**
+  - If changing target framework
+    - Update `NugetTargetMoniker` in `<Library>\tests\<Library>.csproj` (ex: [tests\System.Runtime.csproj](https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System.Runtime.Tests.csproj#L13))


The System.Runtime.Tests.csproj example has a default NugetTargetMoniker of 1.5. Why not 1.7?

<NugetTargetMoniker Condition="'$(NugetTargetMoniker)'==''">.NETStandard,Version=v1.5</NugetTargetMoniker>

The System.Runtime.Tests.csproj example has a default NugetTargetMoniker of 1.5. Why not 1.7?

👍
Are there any existing projects targeting 1.7? They all seem to target 1.3 or 1.5, even after being updated to test a 1.7 assembly. I've tried targeting 1.7 from the Sockets tests, and had little luck getting it to work.

It does support netstandard1.7 see (https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System.Runtime.Tests.builds#L7). The default isn't pointing at that right now but that is something @joperezr is working on updating across the repo.

hughbe · 2016-09-25T14:29:17Z

Documentation/coding-guidelines/adding-api-guidelines.md

+##FAQ
+_**<a name="isnetstandard">Is your API part of netstandard?</a>**_
+
+- For netstandard2.0 refer to https://github.com/dotnet/standard/tree/master/netstandard/ref.


Broken link

This repo was just made public today so the link should work now.

hughbe · 2016-09-25T14:30:06Z

Documentation/coding-guidelines/adding-api-guidelines.md

+      - Update the ProjectReferences in the respective pkgproj's to align with the build configurations in the
+      `<Library>\src\<Library>.builds` file. This may entail adding new references or removing references according to whatever changes were made to the .builds file.
+  - If assembly or package version is updated the package index needs to be updated by running
+    msbuild `<Library>/pkg/<Library>.pkgproj /t:UpdatePackageIndex`


Shouldn't "msbuild" be include in the code snippet

hughbe · 2016-09-25T14:33:24Z

Documentation/coding-guidelines/adding-api-guidelines.md

+**Update tests**
+  - If changing target framework
+    - Update `NugetTargetMoniker` in `<Library>\tests\<Library>.csproj` (ex: [tests\System.Runtime.csproj](https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System.Runtime.Tests.csproj#L13))
+    - Update framework section in `<Library>\tests\project.json` (ex: [System.Runtime\tests\project.json](https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/project.json#L30))


Nit: you need to do this not just for "Changing target framework", but also for adding a new one (e.g. netcoreapp1.1)

CIPop · 2016-09-26T17:51:04Z

This is resolving #11712.

Thanks @weshaggard !

danmoseley · 2016-09-26T20:37:29Z

cc @joperezr again

joperezr

Overall it looks good to me, just left a couple of comments.

joperezr · 2016-09-27T15:28:17Z

Documentation/coding-guidelines/adding-api-guidelines.md

+- If targeting netstandard
+  - Ensure minor version of the library is bumped since last stable package release
+- If targeting netcoreapp
+  - No version bump necessary


I'm not sure I understand why no version bump is needed when the new API is only exposed by netcoreapp

joperezr · 2016-09-27T15:30:37Z

Documentation/coding-guidelines/adding-api-guidelines.md

+          that we want to update outside of servicing events.
+  - Make your code changes.
+
+**Update pkg**


Should we at least mention in here what to do when we have a <Library>/ref/<Library>.builds file? That is a new concept that not everybody might be familiar with.

joperezr · 2016-09-27T15:33:23Z

Oh and actually one more thing: We didn't mention here anything regarding the package harvesting, is that intentional? I ask this because it is one 'magic' thing that happens in some libraries and a lot of people ask about.

AlexGhiondea · 2016-09-28T17:20:10Z

One of the things I ran into was that when you are moving a type from an assembly into a 'lower level' assembly you need to introduce a type-forward to that type from the original assembly.

weshaggard · 2016-10-08T06:15:51Z

I've not forgotten about this PR, just haven't had time to respond to all the feedback. I will try to do it next week.

Note-to-self Update https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/branching-guide.md as well.

karelz · 2016-10-15T16:49:44Z

@weshaggard is it ready for merge? Even if it is not perfect, let's get it in and then we can continue on additional improvements.

Also removed the dead information about APIs only going into future branch

weshaggard · 2016-10-17T18:32:47Z

Thanks everyone I've updated the docs based on the feedback provided. If there is still further clarification needed or updates people would like to see, please file an issue or submit a PR.

Add documentation guidelines for adding new APIs Commit migrated from dotnet/corefx@fa0ee52

dnfclas added the cla-already-signed label Sep 24, 2016

danmoseley suggested changes Sep 25, 2016

View reviewed changes

danmoseley reviewed Sep 25, 2016

View reviewed changes

hughbe reviewed Sep 25, 2016

View reviewed changes

karelz added documentation Documentation bug or enhancement, does not impact product or test code area-Meta labels Sep 26, 2016

joperezr reviewed Sep 27, 2016

View reviewed changes

weshaggard mentioned this pull request Sep 28, 2016

Port the majority of the System.ComponentModel APIs into the open #12081

Merged

hughbe mentioned this pull request Sep 29, 2016

Updating the 'System.Runtime.Extensions' contracts to include the new single-precision Math APIs. #12183

Merged

weshaggard mentioned this pull request Sep 30, 2016

Move S.IO.FileAccess to corelib. #12206

Merged

karelz assigned weshaggard Oct 10, 2016

Add documentation guidelines for adding new APIs

2fd6a60

Also removed the dead information about APIs only going into future branch

weshaggard force-pushed the AddingAPIDocs branch from 5ce5289 to 2fd6a60 Compare October 17, 2016 18:31

weshaggard merged commit fa0ee52 into dotnet:master Oct 17, 2016

weshaggard deleted the AddingAPIDocs branch November 9, 2016 17:48

karelz modified the milestone: 1.2.0 Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#12036 from weshaggard/AddingAPIDocs

b05924d

Add documentation guidelines for adding new APIs Commit migrated from dotnet/corefx@fa0ee52


		##Determining versions and targets

		1. [Determine what library](#determine-what-library) the API goes into.


		_What is the difference between being part of netstandard and building against netstandard?_

		Things that are part of netstandard can only change when we release a new version of a platform

Add documentation guidelines for adding new APIs #12036

Add documentation guidelines for adding new APIs #12036

Uh oh!

Conversation

weshaggard commented Sep 24, 2016

Uh oh!

davidsh commented Sep 24, 2016

Uh oh!

danmoseley commented Sep 25, 2016

Uh oh!

danmoseley left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley Sep 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CIPop commented Sep 26, 2016

Uh oh!

danmoseley commented Sep 26, 2016

Uh oh!

joperezr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Sep 27, 2016

Uh oh!

AlexGhiondea commented Sep 28, 2016

Uh oh!

weshaggard commented Oct 8, 2016

Uh oh!

karelz commented Oct 15, 2016

Uh oh!

danmoseley Sep 25, 2016 •

edited

Loading