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

Refactored FrameworkCompatibilityService for use in NuGet.Jobs #9733

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

advay26
Copy link
Member

@advay26 advay26 commented Nov 15, 2023

We want to reuse the FrameworkCompatibilityService in the NuGet.Jobs repo to get packages' computed frameworks in the Search index-builder jobs. To make this code easier to reuse, I made it a static class. I also updated all its existing references and tests.

Part of https://github.com/NuGet/Engineering/issues/4979

@advay26 advay26 requested a review from a team as a code owner November 15, 2023 02:02
@erdembayar
Copy link
Contributor

To make this code easier to reuse

Can you please explain this part? How exactly does it make it easier? Isn't using as it's almost the same as this, except for some dependency injection and constructor parameters?

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

Seems fine to me. If we get to a point where we need to pass feature flags, configuration, or other services in as static method parameters, then we'll know it should move back to being a non-static service acquired via DI.

@advay26
Copy link
Member Author

advay26 commented Nov 17, 2023

To make this code easier to reuse

Can you please explain this part? How exactly does it make it easier? Isn't using as it's almost the same as this, except for some dependency injection and constructor parameters?

@erdembayar You're probably right that we could do it that way too. I don't have much context on how to add dependency injection here, so that was part of my motivation for doing it this way.

However, I also didn't see any reason why this service couldn't just be static. All its other members were already static, and this class doesn't need to store any state.

@advay26 advay26 requested a review from erdembayar November 17, 2023 02:05
@advay26 advay26 merged commit bc567a1 into dev Nov 17, 2023
1 check passed
@advay26 advay26 deleted the advay26-refactor-compatservice branch November 17, 2023 18:13
@advay26 advay26 mentioned this pull request Dec 13, 2023
11 tasks
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.

4 participants