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

Use Valkyrie::ID<=>String equality #4264

Merged
merged 2 commits into from
Mar 23, 2020
Merged

Use Valkyrie::ID<=>String equality #4264

merged 2 commits into from
Mar 23, 2020

Conversation

no-reply
Copy link
Contributor

@no-reply no-reply commented Feb 7, 2020

Valkyrie 2.1.0 overhauled ID equality as a feature-flag change, with the new
behavior to become default in 3.0.0. Turning it on preemptively for Hyrax will
save us and adopters a lot of trouble going forward.

@samvera/hyrax-code-reviewers

Valkyrie 2.1.0 overhauld ID equality as a feature-flag change, with the new
behavior to become default in 3.0.0. Turning it on preemtively for Hyrax will
save us and adopters a lot of trouble going forward.
@no-reply
Copy link
Contributor Author

no-reply commented Feb 7, 2020

Hold this pending a resolution to samvera/valkyrie#820

@no-reply no-reply changed the title Use Valkyrie::ID<=>String equality WIP: Use Valkyrie::ID<=>String equality Feb 7, 2020
@jeremyf
Copy link
Contributor

jeremyf commented Feb 7, 2020

I like! 👍

@no-reply
Copy link
Contributor Author

no-reply commented Feb 8, 2020

do you think we need to hold this until a release with some fix for samvera/valkyrie#820, or should we roll forward now and let the valkyrie fix come in its own time?

@jeremyf
Copy link
Contributor

jeremyf commented Feb 8, 2020

@no-reply Given the caveat that you posted about (A == B) == true but (B == A) == false I think we should hold.

@stale
Copy link

stale bot commented Mar 9, 2020

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

@stale stale bot added the stale label Mar 9, 2020
@no-reply
Copy link
Contributor Author

no-reply commented Mar 9, 2020

bumping this

@stale stale bot removed the stale label Mar 9, 2020
@no-reply no-reply removed the question label Mar 12, 2020
@no-reply no-reply changed the title WIP: Use Valkyrie::ID<=>String equality Use Valkyrie::ID<=>String equality Mar 12, 2020
Copy link
Contributor

@jeremyf jeremyf left a comment

Choose a reason for hiding this comment

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

Given that Hyrax requires Valkyrie ~> 2.1, this will work. However, since the upstream commit is thusfar only in 2.1.1, I believe the gemspec should be altered to reflect that range. Otherwise, we're relying on an implementer to run bundle update valkyrie on their branch.

I'm adding a commit to reflect that.

For this PR to work for upstream adopters, we need to ensure that our
gemspec includes the [upstream Valkyrie commit][1].

From the valkyrie repository, I ran:

```console
$ git tag --contains 343656c78411cc0788a8f723f8332d2e697213fe
=> v2.1.1
```

[1]:samvera/valkyrie@343656c
@jeremyf
Copy link
Contributor

jeremyf commented Mar 23, 2020

@no-reply I added another commit to this. For the record, I approve of your commit.

Narrowing Valkyrie dependency range

d102a03

For this PR to work for upstream adopters, we need to ensure that our
gemspec includes the upstream Valkyrie commit.

From the valkyrie repository, I ran:

$ git tag --contains 343656c78411cc0788a8f723f8332d2e697213fe
=> v2.1.1

@cjcolvar cjcolvar merged commit ff74cfe into master Mar 23, 2020
@cjcolvar cjcolvar deleted the valkyrie-id-eql branch March 23, 2020 14:49
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.

3 participants