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

Add Bundler v1 Deprecation Warning #10421

Merged
merged 7 commits into from
Aug 15, 2024

Conversation

kbukum1
Copy link
Contributor

@kbukum1 kbukum1 commented Aug 12, 2024

What are you trying to accomplish?

The goal of this PR is to enhance the file parser to return the package_manager information directly. This change has been implemented for Bundler, and other package managers can have this feature added in the future. This information will be used to check for deprecation warnings specifically for Bundler v1.

What issues does this affect or fix?

This change allows us to better handle deprecation warnings for Bundler v1 by providing a consistent way to access package manager information directly from the file parser.

Anything you want to highlight for special attention from reviewers?

  • The abstract package_manager method has been added to the base file parser.
  • For Bundler, the package_manager method has been implemented and returns the necessary information for deprecation warnings.
  • For other package managers, the package_manager method is currently not implemented but can be extended in the future.
  • Ensure to review the implementation of the abstract method and its integration with the Bundler package manager.

How will you know you've accomplished your goal?

  • The file parser will now return the package_manager information directly.
  • The Bundler implementation correctly returns the package manager information.
  • Deprecation warnings for Bundler v1 can be effectively checked using this new context.
  • Tests should confirm that the new functionality works correctly and does not break existing functionality.

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.

@github-actions github-actions bot added the L: ruby:bundler RubyGems via bundler label Aug 12, 2024

abstract!

# The name of the package manager (e.g., "bundler")
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if as part of this effort we make these constants too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am currently keeping it as constants in bundler/package_manager. Do you mean something different since you are pointing in PackageManagerBase? Something like enum types?

next false if dep.previous_requirements.nil?

dep_previous_requirements = dep.previous_requirements
next false if dep_previous_requirements.nil?
Copy link
Member

Choose a reason for hiding this comment

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

It seems like some unrelated changes like these and the types for all the methods have gotten into this PR? Try to keep extra changes out since it's already quite large of a change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is just using variable to solve sorbet issue regarding dep.previous_requirements being not empty when passing to the dependency below. `requirements: dep.previous_requirements. If we don't use that we need to use T.must to tell the sorbet that it is not going to be empty.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why we needed to fully type this file, it's better to keep a PR focussed on doing one thing. Often typing a file can cause unrelated problems. Partially typing new methods is preferred in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just replaced it to make it T.must since above we already have nil check.

@kbukum1 kbukum1 force-pushed the kamil/add_bundler_v1_deprecation_warning branch 2 times, most recently from c87d344 to 9fd9bcd Compare August 13, 2024 17:53
@kbukum1 kbukum1 force-pushed the kamil/add_bundler_v1_deprecation_warning branch 2 times, most recently from bc3dce1 to 6a673a5 Compare August 13, 2024 19:57
@kbukum1 kbukum1 marked this pull request as ready for review August 13, 2024 22:43
@kbukum1 kbukum1 requested a review from a team as a code owner August 13, 2024 22:43
@kbukum1 kbukum1 force-pushed the kamil/add_bundler_v1_deprecation_warning branch 2 times, most recently from e6a6ea7 to 5354eed Compare August 14, 2024 01:21
common/lib/dependabot/package_manager.rb Outdated Show resolved Hide resolved

abstract!

# The name of the package manager (e.g., "bundler")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am currently keeping it as constants in bundler/package_manager. Do you mean something different since you are pointing in PackageManagerBase? Something like enum types?

next false if dep.previous_requirements.nil?

dep_previous_requirements = dep.previous_requirements
next false if dep_previous_requirements.nil?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is just using variable to solve sorbet issue regarding dep.previous_requirements being not empty when passing to the dependency below. `requirements: dep.previous_requirements. If we don't use that we need to use T.must to tell the sorbet that it is not going to be empty.

common/lib/dependabot/shared_helpers.rb Outdated Show resolved Hide resolved
bundler/lib/dependabot/bundler/package_manager.rb Outdated Show resolved Hide resolved
common/lib/dependabot/notices.rb Outdated Show resolved Hide resolved
common/lib/dependabot/package_manager.rb Outdated Show resolved Hide resolved
common/lib/dependabot/notices.rb Outdated Show resolved Hide resolved
@kbukum1 kbukum1 force-pushed the kamil/add_bundler_v1_deprecation_warning branch 5 times, most recently from be87895 to 4268800 Compare August 14, 2024 21:33
Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

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

Nice work!

You can run a CLI job with a Bundler 1 repo and the experiment enabled as a sanity test, if you haven't already.

@kbukum1
Copy link
Contributor Author

kbukum1 commented Aug 15, 2024

Nice work!

You can run a CLI job with a Bundler 1 repo and the experiment enabled as a sanity test, if you haven't already.

Thanks this is be my first time for for enabling experience.

@kbukum1 kbukum1 force-pushed the kamil/add_bundler_v1_deprecation_warning branch from 4268800 to a375f7c Compare August 15, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: ruby:bundler RubyGems via bundler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants