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

simplify some linq expressions #6196

Conversation

SimonCropp
Copy link
Contributor

Bug

Fixes:

Description

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@SimonCropp SimonCropp requested a review from a team as a code owner December 16, 2024 11:56
@microsoft-github-policy-service microsoft-github-policy-service bot added the Community PRs created by someone not in the NuGet team label Dec 16, 2024
@nkolev92 nkolev92 self-assigned this Dec 18, 2024
@SimonCropp SimonCropp changed the title simplify some Where+Count into only Count simplify some linq expressions Dec 20, 2024
@nkolev92
Copy link
Member

Changes look good, there's some formatting issues the CI is reporting.

Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

The LINQ changes are excellent. The other changes are unnecessary 😁

Log = logger,
CacheContext = new SourceCacheContext()
};
var restoreContext = new RestoreArgs() {Sources = new List<string>() {packageSource.FullName}, GlobalPackagesFolder = packagesDir.FullName, Log = logger, CacheContext = new SourceCacheContext()};
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this change, I think it's much easier to read one property per line. It's also unrelated to the title of this PR.

Log = logger,
CacheContext = new SourceCacheContext()
};
var restoreContext = new RestoreArgs() {Sources = new List<string>() {packageSource.FullName}, GlobalPackagesFolder = packagesDir.FullName, Log = logger, CacheContext = new SourceCacheContext()};
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this change, I think it's much easier to read one property per line. It's also unrelated to the title of this PR.

Log = logger,
CacheContext = new SourceCacheContext()
};
var restoreContext = new RestoreArgs() {Sources = new List<string>() {packageSource.FullName}, GlobalPackagesFolder = packagesDir.FullName, Log = logger, CacheContext = new SourceCacheContext()};
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this change, I think it's much easier to read one property per line. It's also unrelated to the title of this PR.

Log = logger,
CacheContext = new SourceCacheContext()
};
var restoreContext = new RestoreArgs() {Sources = new List<string>() {packageSource.FullName}, GlobalPackagesFolder = packagesDir.FullName, Log = logger, CacheContext = new SourceCacheContext()};
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this change, I think it's much easier to read one property per line. It's also unrelated to the title of this PR.

Log = logger,
CacheContext = new SourceCacheContext()
};
var restoreContext = new RestoreArgs() {Sources = new List<string>() {packageSource.FullName}, GlobalPackagesFolder = packagesDir.FullName, Log = logger, CacheContext = new SourceCacheContext()};
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this change, I think it's much easier to read one property per line. It's also unrelated to the title of this PR.

@SimonCropp SimonCropp closed this Dec 25, 2024
@nkolev92
Copy link
Member

nkolev92 commented Jan 7, 2025

@SimonCropp do you care if I reopen this PR and push some changes to your branch (assuming your fork allows it) and drive this PR to completion?

I think simplification changes are worth merging, and this way you'd get credit for the PR.

Otherwise, I'd probably just recreate the PR from my own branch.

@SimonCropp
Copy link
Contributor Author

SimonCropp commented Jan 7, 2025

@nkolev92 go for it. and i dont need credit. just make the changes in whatever way is easiest for you.

I only closed all my PRs since i found the NuGet.Client project to have too much unecessary friction to contribute to

@SimonCropp
Copy link
Contributor Author

@nkolev92 if u want a look at a MS project that is easier to contribute to see testfx.
https://github.com/microsoft/testfx/pulls?q=is%3Apr+author%3ASimonCropp+is%3Aclosed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants