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

Test PR to test copilot reviews #4212

Closed
wants to merge 3 commits into from

Conversation

premun
Copy link
Member

@premun premun commented Dec 4, 2024

No description provided.

@premun premun requested a review from Copilot December 4, 2024 17:00
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 11 changed files in this pull request and generated 6 suggestions.

Files not reviewed (6)
  • src/Microsoft.DotNet.Darc/DarcLib/ILocalGitRepo.cs: Evaluated as low risk
  • src/Microsoft.DotNet.Darc/DarcLib/LocalGitRepo.cs: Evaluated as low risk
  • src/Microsoft.DotNet.Darc/DarcLib/Models/VirtualMonoRepo/SourceManifest.cs: Evaluated as low risk
  • src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CloneManager.cs: Evaluated as low risk
  • src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/RepositoryCloneManager.cs: Evaluated as low risk
  • src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCloneManager.cs: Evaluated as low risk
Comments skipped due to low confidence (2)

src/Microsoft.DotNet.Darc/Darc/Operations/GetChannelsOperation.cs:53

  • Array literals should be created using the 'new' keyword. Use 'new string[] { "diff", "--name-only", "--diff-filter=U", "--relative" }' instead.
result = await vmr.RunGitCommandAsync(["diff", "--name-only", "--diff-filter=U", "--relative"]);

src/Microsoft.DotNet.Darc/Darc/Operations/GetChannelsOperation.cs:83

  • Array literals should be created using the 'new' keyword. Use 'new string[] { "checkout", "--ours", file }' instead.
await vmr.RunGitCommandAsync(["checkout", "--ours", file]);

try
var vmr = _localGitRepoFactory.Create(_vmrInfo.VmrPath);
await vmr.CheckoutAsync(baseBranch);
var result = await vmr.RunGitCommandAsync(["merge", "--no-commit", "--no-ff", targetBranch]);
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Array literals should be created using the 'new' keyword. Use 'new string[] { "merge", "--no-commit", "--no-ff", targetBranch }' instead.

Suggested change
var result = await vmr.RunGitCommandAsync(["merge", "--no-commit", "--no-ff", targetBranch]);
var result = await vmr.RunGitCommandAsync(new string[] { "merge", "--no-commit", "--no-ff", targetBranch });

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
private async Task TryResolvingSourceManifestConflict(ILocalGitRepo vmr, string mappingName)
{
// We load the source manifest from the target branch and replace the current mapping (and its submodules) with our branches' information
var result = await vmr.RunGitCommandAsync(["show", "MERGE_HEAD:" + VmrInfo.DefaultRelativeSourceManifestPath]);
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Array literals should be created using the 'new' keyword. Use 'new string[] { "show", "MERGE_HEAD:" + VmrInfo.DefaultRelativeSourceManifestPath }' instead.

Suggested change
var result = await vmr.RunGitCommandAsync(["show", "MERGE_HEAD:" + VmrInfo.DefaultRelativeSourceManifestPath]);
var result = await vmr.RunGitCommandAsync(new string[] { "show", "MERGE_HEAD:" + VmrInfo.DefaultRelativeSourceManifestPath });

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options

_fileSystem.WriteToFile(_vmrInfo.SourceManifestPath, theirSourceManifest.ToJson());
_sourceManifest.Refresh(_vmrInfo.SourceManifestPath);
await vmr.StageAsync([_vmrInfo.SourceManifestPath]);
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Array literals should be created using the 'new' keyword. Use 'new string[] { _vmrInfo.SourceManifestPath }' instead.

Suggested change
await vmr.StageAsync([_vmrInfo.SourceManifestPath]);
await vmr.StageAsync(new string[] { _vmrInfo.SourceManifestPath });

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
var prBranch = "OutOfOrderMergesTest-ff";

_vmrInfo.VmrPath = new NativePath(path);
await _cloneManager.PrepareVmrAsync([path], [targetBranch, prBranch], prBranch, default);
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Array literals should be created using the 'new' keyword. Use 'new string[] { path }' and 'new string[] { targetBranch, prBranch }' instead.

Suggested change
await _cloneManager.PrepareVmrAsync([path], [targetBranch, prBranch], prBranch, default);
await _cloneManager.PrepareVmrAsync(new string[] { path }, new string[] { targetBranch, prBranch }, prBranch, default);

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
{
// Write out a simple list of each channel's name
foreach (var channel in allChannels.OrderBy(c => c.Name))
var path = @"C:\Users\prvysoky\AppData\Local\Temp\_vmrTests\wvzmghdz.ec2\_tests\xeihkfov.lgk\vmr";
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Hardcoded paths should be avoided in production code. Consider using a configuration setting or environment variable instead.

Suggested change
var path = @"C:\Users\prvysoky\AppData\Local\Temp\_vmrTests\wvzmghdz.ec2\_tests\xeihkfov.lgk\vmr";
var path = Environment.GetEnvironmentVariable("VmrPath") ?? @"C:\Users\prvysoky\AppData\Local\Temp\_vmrTests\wvzmghdz.ec2\_tests\xeihkfov.lgk\vmr";

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@@ -172,6 +173,8 @@ public virtual IServiceCollection RegisterServices(IServiceCollection services)
);
services.TryAddSingleton<IRemoteTokenProvider>(_ => new RemoteTokenProvider(AzureDevOpsPat, GitHubPat));
services.TryAddSingleton<ICommandLineOptions>(_ => this);
services.AddTransient<CodeFlowConflictResolver>();
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

The lifetime of CodeFlowConflictResolver should be changed to scoped to match the lifetime of IVmrCloneManager, as CodeFlowConflictResolver depends on IVmrCloneManager.

Suggested change
services.AddTransient<CodeFlowConflictResolver>();
services.AddScoped<CodeFlowConflictResolver>();

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
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.

1 participant