Refactor and update sample project build. #3816
Conversation
Scope: Make samples build with nor or minimal changes to actual snippets usied in docs. Added namespaces to organize code and prevent naming conflicts. Used filename as namespace, and for files with names of classes, added "CS" to namespace Adjusted `using` directives to include necessary references to make build pass. Wrapped code snippets in conditional compilation directives for mostly .NET Framework-specific APIs. Made some changes to the .csproj file to enable local "dotnet build" - happy to revert as needed.
There was a problem hiding this comment.
Pull request overview
This PR refactors sample code files to enable building with minimal changes to documentation snippets. The primary changes include adding namespace declarations to prevent naming conflicts and wrapping .NET Framework-specific code in conditional compilation directives.
- Adds unique namespaces to all sample files, typically using the filename as the namespace name (with "CS" suffix for files with class names)
- Wraps .NET Framework-specific code in
#if NETFRAMEWORKdirectives - Adds missing
usingdirectives required for compilation - Updates project configuration to support multi-targeting (net8.0 and net462)
Reviewed changes
Copilot reviewed 228 out of 228 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| TransactionIsolationLevels.cs | Changed namespace from generic to filename-based |
| SqlVectorExample.cs | Added namespace, missing using directives, and changed async to sync method call |
| SqlUserDefinedType*.cs | Added namespaces and changed namespace references from Microsoft.Data.SqlClient.Server to Microsoft.SqlServer.Server |
| SqlRowUpdatingEventArgs.cs, SqlParameterCollection_*.cs, SqlDataAdapter_SelectCommand.cs, etc. | Wrapped .NET Framework-specific code in conditional compilation directives |
| SqlDataReader_DataDiscoveryAndClassification.cs | Fixed typo in Console method name |
| SqlDataAdapter_Properties.cs | Removed nested namespace and unnecessary using directive |
| SqlCommand_ExecuteNonQuery_SP_DML.cs | Removed unused variable declaration |
| SqlDataAdapter_SPIdentityReturn.cs | Changed Int to int for type consistency |
| SqlConfigurableRetryLogic_SqlCommand.cs | Added wrapper classes to separate code snippets |
| SqlClientEventSource.cs, SqlClientDiagnosticCounter.cs | Added namespaces and missing using directives |
| RegisterCustomKeyStoreProvider_*.cs | Added namespaces, using directives, and stub implementations for custom providers |
| SqlUserDefinedTypeAttribute_Type1.cs | Fixed typo in Convert.Toint method calls |
| Microsoft.Data.SqlClient.Samples.csproj | Added multi-targeting configuration and package reference |
| ConnectionStrings_Encrypt.cs | Renamed parameter for clarity |
| @@ -1,4 +1,5 @@ | |||
| using System; | |||
| /* | |||
There was a problem hiding this comment.
Excluded this from build, as it contains obsolete APIs
There was a problem hiding this comment.
Can you add a comment to this effect?
Also, if you use #if false instead of the block comment /* */ then we still get nice IDE presentation and you can put comments about what isn't compiling adjacent to the offending lines.
| @@ -1,4 +1,8 @@ | |||
| using System; | |||
| /* | |||
There was a problem hiding this comment.
Could not make this build, so commented out
| } | ||
| // <Snippet1> | ||
| static void ToggleConfigEncryption(string exeFile) | ||
| static void ToggleConfigEncryption(string exeConfigName) |
There was a problem hiding this comment.
Corrected parameter name to make build pass
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net8.0;net462</TargetFrameworks> |
There was a problem hiding this comment.
Added to make "dotnet build" work
There was a problem hiding this comment.
Should we target .NET 10 here instead of 8?
| <PackageReference Include="Microsoft.Data.SqlClient" /> | ||
| <PackageReference Include="Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider" /> | ||
| <PackageReference Include="Newtonsoft.Json" /> | ||
| <PackageReference Include="System.Configuration.ConfigurationManager" /> |
There was a problem hiding this comment.
Added to make build pass
|
|
||
| <!-- .NET Framework references --> | ||
| <ItemGroup Condition="$(TargetGroup) == 'netfx'"> | ||
| <ItemGroup Condition="'$(TargetFramework)' == 'net462'"> |
There was a problem hiding this comment.
Added to make local "dotnet build" work
| Point pt = new Point(); | ||
| pt.X = Convert.Toint(xy[0]); | ||
| pt.Y = Convert.Toint(xy[1]); | ||
| pt.X = Convert.ToInt32(xy[0]); |
There was a problem hiding this comment.
Fix of incorrect sample code
| class Program | ||
| { | ||
| static void Main() | ||
| static void Main(string connectionString) |
There was a problem hiding this comment.
Added missing parameter to make build pass
| { | ||
| customKeyStoreProviders.Clear(); | ||
| SqlColumnEncryptionAzureKeyVaultProvider azureKeyVaultProvider = new SqlColumnEncryptionAzureKeyVaultProvider(); | ||
| SqlColumnEncryptionAzureKeyVaultProvider azureKeyVaultProvider = new SqlColumnEncryptionAzureKeyVaultProvider(new DefaultAzureCredential()); |
There was a problem hiding this comment.
Uses the updated API to make build pass
| } | ||
| // </Snippet1> | ||
|
|
||
| class MyCustomKeyStoreProvider : SqlColumnEncryptionKeyStoreProvider |
There was a problem hiding this comment.
Added dummy class to make build pass (not part of snippet)
| class Program | ||
| { | ||
| static void Main() | ||
| static void Main(string connectionString) |
There was a problem hiding this comment.
added missing parameter to make build pass
| { | ||
| customKeyStoreProviders.Clear(); | ||
| SqlColumnEncryptionAzureKeyVaultProvider azureKeyVaultProvider = new SqlColumnEncryptionAzureKeyVaultProvider(); | ||
| SqlColumnEncryptionAzureKeyVaultProvider azureKeyVaultProvider = new SqlColumnEncryptionAzureKeyVaultProvider(new DefaultAzureCredential()); |
There was a problem hiding this comment.
Use the updated API to make build pass
| } | ||
| // </Snippet1> | ||
|
|
||
| class MyCustomKeyStoreProvider : SqlColumnEncryptionKeyStoreProvider |
There was a problem hiding this comment.
Added dummy class to make build pass (not part of snippet)
| { | ||
| // Create a new SqlColumnEncryptionAzureKeyVaultProvider with the user's credentials and save it for future use | ||
| azureKeyVaultProvider = new SqlColumnEncryptionAzureKeyVaultProvider(); | ||
| azureKeyVaultProvider = new SqlColumnEncryptionAzureKeyVaultProvider(new DefaultAzureCredential()); |
There was a problem hiding this comment.
Use the updated API to make build pass
| { | ||
| // Create a new SqlColumnEncryptionAzureKeyVaultProvider with the user's credentials and save it for future use | ||
| azureKeyVaultProvider = new SqlColumnEncryptionAzureKeyVaultProvider(); | ||
| azureKeyVaultProvider = new SqlColumnEncryptionAzureKeyVaultProvider(new DefaultAzureCredential()); |
There was a problem hiding this comment.
Use the updated API to make build pass
| @@ -1,3 +1,4 @@ | |||
| /* This does not compile, as multiple methods are missing. | |||
There was a problem hiding this comment.
Removed for build, as this is just code fragments
| { | ||
| using (SqlConnection connection = new SqlConnection(connectionString)) | ||
| { | ||
| SqlCommand command = new SqlCommand(connection); |
There was a problem hiding this comment.
Removed to make build pass
| cmd.ExecuteNonQuery(); | ||
| } | ||
| // </Snippet2> | ||
| private class RetryCommandSample2 |
There was a problem hiding this comment.
Added multiple classes to avoid name clashes
| await cmd.ExecuteNonQueryAsync(); | ||
| } | ||
| // </Snippet3> | ||
| } |
There was a problem hiding this comment.
Added multiple classes to avoid name clashes (outside of snippet)
| } | ||
|
|
||
| namespace CSDataAdapterOperations.Properties | ||
| internal sealed partial class Settings : System.Configuration.ApplicationSettingsBase |
There was a problem hiding this comment.
Removed namespace to make code build.
|
|
||
| // Retrieve the ReturnValue. | ||
| Int rowCount = (Int)adapter.InsertCommand.Parameters["@RowCount"].Value; | ||
| int rowCount = (int)adapter.InsertCommand.Parameters["@RowCount"].Value; |
There was a problem hiding this comment.
Fix typo to make code build
| } | ||
| } | ||
| Console.Writeline($"reader.SensitivityClassification.SensitivityRank : {reader.SensitivityClassification.SensitivityRank.ToString()}"); | ||
| Console.WriteLine($"reader.SensitivityClassification.SensitivityRank : {reader.SensitivityClassification.SensitivityRank.ToString()}"); |
There was a problem hiding this comment.
Fix typo to make code build
| cmd.Parameters.Add(p); | ||
|
|
||
| await cmd.PrepareAsync(); | ||
| cmd.Prepare(); |
There was a problem hiding this comment.
Make sample build on NetFX
There was a problem hiding this comment.
Thanks you @ErikEJ ! These are excellent changes that help progress the goal of making our samples legitimate projects that build and function correctly.
I have a few questions for the team regarding some conventions so please hold off making any changes until we get some responses from them.
| using Microsoft.SqlServer.Server; | ||
|
|
||
| namespace test | ||
| namespace DataWorks_IBinarySerialize_Sample |
There was a problem hiding this comment.
Should we come up with a namespace convention for samples? For example, base namespace names of:
Microsoft.SqlServer.Server.Samples
Microsoft.Data.SqlClient.Samples
And then append something unique to make the complete namespace:
Microsoft.SqlServer.Server.Samples.DataWorks.IBinarySerialize
@benrr101 and others - should we discuss before Erik makes any changes here?
| @@ -1,4 +1,6 @@ | |||
| using System.IO; | |||
| namespace DataWorks_SqlFunctionAttribute_Sample; | |||
There was a problem hiding this comment.
I think we prefer to put usings outside of any namespace declaration.
There was a problem hiding this comment.
This is to deliberate minimize impact on sample code - I think mentally this code base is quite different from production code
There was a problem hiding this comment.
This is to deliberate minimize impact on sample code - I think mentally this code base is quite different from production code
| @@ -1,4 +1,5 @@ | |||
| using System; | |||
| /* | |||
There was a problem hiding this comment.
Can you add a comment to this effect?
Also, if you use #if false instead of the block comment /* */ then we still get nice IDE presentation and you can put comments about what isn't compiling adjacent to the offending lines.
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net8.0;net462</TargetFrameworks> |
There was a problem hiding this comment.
Should we target .NET 10 here instead of 8?
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
"Can you add a comment to this effect? Also, if you use #if false instead of the block comment /* */ then we still get nice IDE presentation and you can put comments about what isn't compiling adjacent to the offending lines." Sure. Will adding #if not disrupt the sample code?? |
|
Basically, for any files that did not have a namespace before, adding it to the top of the file is making sure that this does not interfere with any sample code. |
|
@paulmedynski also not sure the samples are included in any build at the moment... |
3 participants
Scope: Make samples build with nor or minimal changes to actual snippets used in docs.
Added namespaces to organize code and prevent naming conflicts. Used filename as namespace, and for files with names of classes, added "CS" to namespace
Adjusted
usingdirectives to include necessary references to make build pass.Wrapped code snippets in conditional compilation directives for mostly .NET Framework-specific APIs.
Made some changes to the .csproj file to enable local "dotnet build" - happy to revert as needed.
I will comment on any other type of changes made to a few samples.
Unsure about the sample for Microsoft.SqlServer.Server, but can revert these if needed. (and not build them)
Related to issue #3725