Skip to content

Conversation

@Nigusu-Allehu
Copy link
Member

@Nigusu-Allehu Nigusu-Allehu commented Feb 6, 2025

Bug

Fixes: NuGet/Home#13975

Addresses comments regarding renaming variables and refactoring test in #6138

Description

Remove IsDesktop & Add PluginFile PluginFile(filePath, state) contractor

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.

@Nigusu-Allehu Nigusu-Allehu self-assigned this Feb 6, 2025
@Nigusu-Allehu Nigusu-Allehu marked this pull request as ready for review February 6, 2025 00:47
@Nigusu-Allehu Nigusu-Allehu requested a review from a team as a code owner February 6, 2025 00:47
@nkolev92
Copy link
Member

nkolev92 commented Feb 6, 2025

PR Naming:
The PR title should say what the PR does, more so than the why.
The issue covers the why.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

I think you need to flip the condition during the rename.

@Nigusu-Allehu
Copy link
Member Author

I think you need to flip the condition during the rename.

Yes that is correct! I think this got a bit confusing because I did not do that. I have also re written the description to make this clearer

@Nigusu-Allehu Nigusu-Allehu changed the title Plugin discovery: Address code quality PR comments Rename IsDesktop to RequiresDotNetHost and refactor PluginDiscoveryTests Feb 6, 2025
@Nigusu-Allehu Nigusu-Allehu changed the title Rename IsDesktop to RequiresDotNetHost and refactor PluginDiscoveryTests Rename IsDesktop to RequiresDotNetHost Feb 6, 2025
nkolev92
nkolev92 previously approved these changes Feb 6, 2025
nkolev92
nkolev92 previously approved these changes Feb 7, 2025
@jeffkl
Copy link
Contributor

jeffkl commented Feb 7, 2025

@Nigusu-Allehu and I spoke offline and thought it would be better to just get rid of IsDesktop/RequiresDotNetHost in favor of a new constructor that does the same thing. It allows a decent amount of duplicated code to be removed and is cleaner in my opinion.

@jebriede
Copy link
Contributor

jebriede commented Feb 7, 2025

@Nigusu-Allehu and I spoke offline and thought it would be better to just get rid of IsDesktop/RequiresDotNetHost in favor of a new constructor that does the same thing. It allows a decent amount of duplicated code to be removed and is cleaner in my opinion.

This is great! @Nigusu-Allehu can we rename the title of this PR to reflect this update? Currently it says that we are renaming IsDesktop to RequiresDotNetHost, but the change is more than just renaming the variable now.

Copy link
Contributor

@jebriede jebriede left a comment

Choose a reason for hiding this comment

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

Please update the title of the PR to reflect the updated changes. The code changes LGTM! 🚀

@Nigusu-Allehu Nigusu-Allehu changed the title Rename IsDesktop to RequiresDotNetHost Remove IsDesktop & Add PluginFile PluginFile(filePath, state) contructor Feb 7, 2025
@Nigusu-Allehu Nigusu-Allehu changed the title Remove IsDesktop & Add PluginFile PluginFile(filePath, state) contructor Remove IsDesktop & Add PluginFile PluginFile(filePath, state) contractor Feb 7, 2025
@Nigusu-Allehu Nigusu-Allehu merged commit 75a3792 into dev Feb 7, 2025
23 of 24 checks passed
@Nigusu-Allehu Nigusu-Allehu deleted the dev-nyenework-plugin-comments branch February 7, 2025 20:43
@Nigusu-Allehu Nigusu-Allehu changed the title Remove IsDesktop & Add PluginFile PluginFile(filePath, state) contractor Remove IsDesktop & Add PluginFile PluginFile(filePath, state) constructor May 5, 2025
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.

Address comments in Implement Support for NuGet Authentication Plugins as .NET Tools PR

5 participants