[PROTOTYPE] Enforce code styles in build - #3144
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
github-actions
Bot
added
tools-ACR
tools-Advisor
tools-Aks
tools-AppConfig
github-actions
Bot
added
tools-Speech
tools-SQL
tools-SreAgent
tools-Storage
tools-Workbooks
labels
vukelich
left a comment
vukelich
left a comment
There was a problem hiding this comment.
Some just questions. Some subjective. A couple actual issues.
| /// <item> | ||
| /// <description> | ||
| /// <see cref="IAzureTokenCredentialProvider"/> via <see cref="Authentication.AuthenticationServiceCollectionExtensions.AddSingleIdentityTokenCredentialProvider"/>. | ||
| /// This can be overridden using <see cref="Authentication.AuthenticationServiceCollectionExtensions.AddAzureTokenCredentialProvider"/> |
There was a problem hiding this comment.
Are the lines about overriding no longer true? I believe I left those clues in the doc comments intentionally when adding this code due to some design issues at app start up.
There was a problem hiding this comment.
AuthenticationServiceCollectionExtensions.AddAzureTokenCredentialProvider doesn't exist as an API anymore, so unless this was something external I missed, this doesn't exist anymore.
| /// <param name="subscription">The subscription ID or name</param> | ||
| /// <param name="retryPolicy">Optional retry policy configuration</param> | ||
| /// <param name="converter">Function to convert JsonElement to the target type</param> | ||
| /// <param name="tableName">Optional table name to query (default: "resources")</param> |
There was a problem hiding this comment.
Do or don't: I'm torn on (default: "resources") in the doc comment.
From a maintainability stance, this can easily diverge from the code. Other places solve this by defining a static string or const string, so that both the code and the doc comment resolve to the same string value. For example, see IOptionsSnapshot<TOptions>.Get(String).
(Aside: If a const string, then the value can be used in the default parameter syntax because its compile-time. If static string, then it must be used in the method body BUT the value is a runtime value not a compiler-time value, which allows for dependent libraries to use the new value without being recompiled. The .NET Options.DefaultName is static.)
On the other hand, the "resources" table name does have direct ARG meaning, so it provides the reader some better understanding of the system.
Though, using that table is the default behavior of the ARG API. We could be fine with a null default value and let ARG's query engine do what it does. Switching to letting ARG handle it would be more scope than this code change wants, but it can be beneficial to reduce our own business logic when it exists downstream already.
There was a problem hiding this comment.
In this PR, I'd say let's leave it as-is, as it documents current behaviors and won't introduce scope creep. But I also agree with this comment, and I'm in the opinion that something this meaningful should require explicit interaction by the caller to indicate the table to query and we provide some canned consts to help. This has been a spot where we've seen a few bugs as the default was used but the needed ARG table was something else (hence why this is a parameter in the first place).
| /// <param name="azureLocation">The Azure location for the resource.</param> | ||
| /// <param name="content">The content to create or update the resource with.</param> | ||
| /// <param name="jsonTypeInfo">The JSON type information for serialization.</param> | ||
| /// <param name="cancellationToken">Cancellation token</param> |
There was a problem hiding this comment.
Minor: A cancellation token. fits a little better since the other parameters are also expressed as sentences.
I won't repeat this comment for each place that it's applicable. I'm sure the repo has inconsistent language for this parameter.
There was a problem hiding this comment.
Update: the language used is inconsistent in the repo. in .NET, and across lines added in this PR. Idk what to suggest or say.
There was a problem hiding this comment.
I may have left this to an agent to decide what to do. I'll work on making this consistent across what we own.
| SetDefaultPollInterval(); | ||
| } | ||
|
|
||
| [Conditional("DEBUG")] |
There was a problem hiding this comment.
This is not necessarily equivalent to the previous code. Using [Conditional(...)] will include or omit invocations to the attached method, but it keeps the method body in the source code. If unused code paths are removed by other compliation configurations, then it'll be similar to what we had before.
What is the aim for this change? Was there an analyzers requiring it?
There was a problem hiding this comment.
The analyzers and IDEs get a bit weird on preprocessor blocks on properly conveying when code is still used. This was making it so they do understand this code does get used. The other option here was to have #pragma statements to disable analyzer rules that flag the code as unused.
No strong opinion on which to use here.
| using Microsoft.Mcp.Core.Areas.Server.Options; | ||
| using Microsoft.Mcp.Core.Commands; | ||
| using Microsoft.Mcp.Core.Extensions; | ||
| using Microsoft.Mcp.Core.Services.ProcessExecution; |
There was a problem hiding this comment.
Doc references
| }); | ||
| } | ||
|
|
||
| #pragma warning disable IDE0060 // Remove unused parameter |
There was a problem hiding this comment.
Why the disable rather than remove string completionNote?
There was a problem hiding this comment.
This ties back to the only usage of the required tool parameter completion-note. This needs a deeper follow up than just deleting it (in my opinion).
2 participants
What does this PR do?
Makes some changes to the build to enforce code style rules while building the project and enabling more diagnostics to be raised.
GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline