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

Metric collection for DotNet ecosystem and languages #11096

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sachin-sandhu
Copy link
Contributor

What are you trying to accomplish?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@sachin-sandhu sachin-sandhu self-assigned this Dec 10, 2024
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Dec 10, 2024
@sachin-sandhu sachin-sandhu force-pushed the ssandhu/nuget-package-manager branch 22 times, most recently from f68ba6d to d1fb58e Compare December 11, 2024 08:45
@sachin-sandhu sachin-sandhu force-pushed the ssandhu/nuget-package-manager branch 3 times, most recently from 20c5cbf to 529ddd2 Compare December 11, 2024 08:58

sig { returns(Ecosystem::VersionManager) }
def package_manager
NugetPackageManager.new(nuget_version)
Copy link
Contributor

@amazimbe amazimbe Dec 11, 2024

Choose a reason for hiding this comment

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

nuget_version could be nil so you probably need T.must(nuget_version)

end

sig { returns(T::Array[Dependabot::Dependency]) }
def content_json
Copy link
Contributor

Choose a reason for hiding this comment

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

Earlier, I run into issues when I changed the public interface of this class. Try moving this into the private section below.

# following block
version = Dependabot::SharedHelpers.run_shell_command("dotnet nuget --version").split("Command Line").last&.strip

it "does not raise error" do
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, this looks like testing https://github.com/dependabot/dependabot-core/pull/11096/files#diff-81366b995ef0ba3d519c0c6bc42b165a1ed2bd2059cc6f51b8fecaa4d80b881fR138 and I feel that's where this test should move to. This class does not run the shell command.

@sachin-sandhu sachin-sandhu force-pushed the ssandhu/nuget-package-manager branch 5 times, most recently from 90deca9 to 53524bf Compare December 11, 2024 14:01
@sachin-sandhu sachin-sandhu force-pushed the ssandhu/nuget-package-manager branch from 53524bf to 1727bc7 Compare December 11, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants