Bundler 2: [Prerelease] Implement v2 native functions for LatestVersionFinder #3327
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
feelepxyz
reviewed
Mar 23, 2021
There was a problem hiding this comment.
Sweet! Do all specs pass with v2? I reckon we should get this in so I can remove the pending specs in https://github.com/dependabot/dependabot-core/pull/3319/files
Thoughts on doing a follow up PR to move to project based fixtures?
feelepxyz
reviewed
Mar 23, 2021
| @@ -0,0 +1,86 @@ | |||
| module Functions | |||
There was a problem hiding this comment.
Could also copy these native helper specs over for v2 https://github.com/dependabot/dependabot-core/blob/main/bundler/helpers/v1/spec/functions/dependency_source_spec.rb
|
@feelepxyz Yeah, that sounds good, I'll add the native helper specs I missed and make sure those pass, then we should merge I think |
brrygrdn
force-pushed
the
brrygrdn/bundler-v2-latest-version-finder
branch
from
03d481a
to
07786ca
Compare
2 tasks
feelepxyz
reviewed
Mar 23, 2021
brrygrdn
force-pushed
the
brrygrdn/bundler-v2-latest-version-finder
branch
2 times, most recently
from
30b73e4
to
9351019
Compare
brrygrdn
commented
Mar 23, 2021
feelepxyz
reviewed
Mar 23, 2021
bundler/spec/dependabot/bundler/update_checker/latest_version_finder_spec.rb
Show resolved
Hide resolved
|
The NPM test failures do seem to be local to this branch, I suspect the change to Dependabot::Error might be involved. Update: Confirmed, looking at reverting that change and making an alternative fix for basic auth. |
brrygrdn
force-pushed
the
brrygrdn/bundler-v2-latest-version-finder
branch
from
7d7bdfc
to
c27be08
Compare
Merged
jeffwidman
added a commit
that referenced
this pull request
Nov 22, 2022
Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` comment, but that accidentally got overlooked. So this removes them.
jeffwidman
added a commit
that referenced
this pull request
Nov 23, 2022
Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` comment, but that accidentally got overlooked. So this removes them.
jeffwidman
added a commit
that referenced
this pull request
Jan 23, 2023
Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` comment, but that accidentally got overlooked. So this removes them.
jeffwidman
added a commit
that referenced
this pull request
Jan 23, 2023
Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` comment, but that accidentally got overlooked. So this removes them, and then fixes the failures. One test was straightforward change from method to hash value. But the other was a logical change from expecting a raised error to expecting no error. However, this has been true since this code was originally committed (once #3327 was merged to make the test work): https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250 I considered removing it, but thought it didn't hurt to continue to keep it in to ensure no regressions... it is a `bundler 2` test, so still valid even after we drop support for `bundler 1`.
jeffwidman
added a commit
that referenced
this pull request
Jan 23, 2023
Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` comment, but that accidentally got overlooked. So this removes them, and then fixes the failures. One test was straightforward change from method to hash value. But the other was a logical change from expecting a raised error to expecting no error. However, this has been true since this code was originally committed (once #3327 was merged to make the test work): https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250 I considered removing it, but thought it didn't hurt to continue to keep it in to ensure no regressions... it is a `bundler 2` test, so still valid even after we drop support for `bundler 1`.
jeffwidman
added a commit
that referenced
this pull request
Jan 24, 2023
Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` marker, but that accidentally got overlooked. So this removes the `pending` marker, and then fixes the failures: 1. The first was a straightforward change from method to hash value. 2. The second wasn't raising the expected error... However, this has been true since [this code was originally committed](https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250) (once #3327 was merged to make the test work). So I deleted the test as it added no value.
jeffwidman
added a commit
that referenced
this pull request
Jan 24, 2023
Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` marker, but that accidentally got overlooked. So this removes the `pending` marker, and then fixes the failures: 1. The first was a straightforward change from method to hash value. 2. The second wasn't raising the expected error... However, this has been true since [this code was originally committed](https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250) (once #3327 was merged to make the test work). So I deleted the test as it added no value.
alcere
pushed a commit
that referenced
this pull request
Feb 20, 2023
Per the discussion in #3319 (comment), these only temporarily needed to be marked `pending` until #3327 was merged. From the first thread it looks like the intent was to remove the `pending` marker, but that accidentally got overlooked. So this removes the `pending` marker, and then fixes the failures: 1. The first was a straightforward change from method to hash value. 2. The second wasn't raising the expected error... However, this has been true since [this code was originally committed](https://github.com/dependabot/dependabot-core/pull/3319/files#r600526250) (once #3327 was merged to make the test work). So I deleted the test as it added no value.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds the DependencySource class back into the v2 helpers without any modifications, tests that it works with bundler v2.
Notes
This may be better merging into #3319 since it is a downstream component
TODOs
Update specs to use project-based fixtures.As per comment below, we'll follow up with those separately to unblock #3319