Skip to content

docs: unify style in C# section#128497

Open
geologyrocks wants to merge 2 commits into
MicrosoftDocs:mainfrom
geologyrocks:patch-2
Open

docs: unify style in C# section#128497
geologyrocks wants to merge 2 commits into
MicrosoftDocs:mainfrom
geologyrocks:patch-2

Conversation

@geologyrocks
Copy link
Copy Markdown
Contributor

Maintainers, I couldn't find where the ToDoModel.cs was located, but it also needs changing as it uses a mix of PascalCase and lowercase for its Properties; these should all be PascalCase.

:::code language="csharp" source="~/functions-sql-todo-sample/ToDoModel.cs" range="6-16":::

@prmerger-automator
Copy link
Copy Markdown
Contributor

@geologyrocks : Thanks for your contribution! The author(s) and reviewer(s) have been notified to review your proposed change.

@learn-build-service-prod
Copy link
Copy Markdown
Contributor

Learn Build status updates of commit 4eed1b4:

✅ Validation status: passed

File Status Preview URL Details
articles/azure-functions/functions-bindings-azure-sql-trigger.md ✅Succeeded

For more details, please refer to the build report.

@v-regandowner
Copy link
Copy Markdown
Contributor

@JetterMcTedder

Can you review the proposed changes?

IMPORTANT: When the changes are ready for publication, adding a #sign-off comment is the best way to signal that the PR is ready for the review team to merge.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@prmerger-automator prmerger-automator Bot added the aq-pr-triaged tracking label for the PR review team label May 13, 2026
@ggailey777
Copy link
Copy Markdown
Contributor

Hey @geologyrocks thanks for helping out. That snippet source repo is here: https://github.com/Azure-Samples/azure-sql-binding-func-dotnet-todo/blob/docs-snippets/ToDoModel.cs (If you do create a PR, please LMK and I'll find someone to merge it).

@ggailey777
Copy link
Copy Markdown
Contributor

ggailey777 commented May 18, 2026

GitHub Copilot has some other feedback on your updates:


Thanks for contributing @geologyrocks! I appreciate the intent to unify the casing, and you're right that PascalCase is the correct convention for C# public properties. However, there are a few issues with this PR as-is:

Issues found

1. Compile error — structured logging syntax

The logging changes use $"..." (string interpolation) combined with message template placeholders and extra args:

logger.LogInformation($"Change operation: {ChangeOperation}", change.Operation);

This won't compile because ChangeOperation isn't a variable in scope. It should be either:

  • Structured logging (no $): logger.LogInformation("Change operation: {ChangeOperation}", change.Operation);
  • String interpolation (no extra args): logger.LogInformation($"Change operation: {change.Operation}");

2. Compile error — variable name casing typo

The variable is declared as toDoItem but later referenced as todoItem (lowercase d). C# is case-sensitive, so this won't compile.

3. Java fields should stay camelCase

Java naming conventions use camelCase for fields (order, title, url, completed). PascalCase is a C#/.NET convention and shouldn't be applied to the Java examples.

4. Sample repo needs updating first

The inline code snippets reference properties from the ToDoItem model, which is pulled from the sample repo via:

:::code language="csharp" source="~/functions-sql-todo-sample/ToDoModel.cs" range="6-16":::

The ToDoModel.cs currently still uses lowercase (order, title, url, completed). That file needs to be updated to PascalCase first, then the docs can be updated to match.

Suggested path forward

  1. Submit a PR to Azure-Samples/azure-sql-binding-func-dotnet-todo (branch docs-snippets) to fix ToDoModel.cs property casing to PascalCase
  2. Update this docs PR to fix the C# inline snippets (with correct variable casing and keeping the existing interpolation style for logging)
  3. Revert the Java changes

Let me know if you'd like help with the sample repo PR!

@ggailey777
Copy link
Copy Markdown
Contributor

#label:"changes-required"

@prmerger-automator
Copy link
Copy Markdown
Contributor

@geologyrocks : Thanks for your contribution! The author(s) and reviewer(s) have been notified to review your proposed change.

@geologyrocks
Copy link
Copy Markdown
Contributor Author

Hi all, my ToDoItem PR is here and I've made the changes suggested by Copilot. Apologies for the bad Java change, I'd naïvely assumed it was the same as C#, but as it's outside my wheelhouse I've reverted it.

I decided to keep to the new pattern of passing logging arguments as named properties instead of an interpolated string as it makes it easier to search logs for the named properties instead of interpolated strings. It's also a recommendation in MS docs https://learn.microsoft.com/en-us/dotnet/core/extensions/logging/library-guidance#avoid-string-interpolation-in-logging

Does your repo permit squash commits? I was planning to get this PR closed that way. No problem if not, I'll rebase to squash the two commits to give clean git history.

@geologyrocks geologyrocks changed the title docs: unify style in C# and Java sections docs: unify style in C# section Jun 2, 2026
@learn-build-service-prod
Copy link
Copy Markdown
Contributor

Learn Build status updates of commit e8f36e9:

✅ Validation status: passed

File Status Preview URL Details
articles/azure-functions/functions-bindings-azure-sql-trigger.md ✅Succeeded

For more details, please refer to the build report.

@v-dirichards
Copy link
Copy Markdown
Contributor

@ggailey777 Could you review this proposed update to your article and enter #sign-off in a comment if it's ready to merge?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants