Skip to content

Figure out why we need to override Gem::NameTuple.new#7229

Closed
deivid-rodriguez wants to merge 3 commits intomasterfrom
debug-checksums
Closed

Figure out why we need to override Gem::NameTuple.new#7229
deivid-rodriguez wants to merge 3 commits intomasterfrom
debug-checksums

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

What was the end-user or developer problem that led to this PR?

Just trying to figure out why this is needed because it's very non standard.

What is your fix for the problem, implemented in this PR?

Make sure the following tasks are checked

@segiddins segiddins requested a review from martinemde December 7, 2023 20:48
@martinemde
Copy link
Copy Markdown
Contributor

For old rubygems that don't to_s their platform arg (which we can't easily test here).

Copy link
Copy Markdown
Contributor

@martinemde martinemde left a comment

Choose a reason for hiding this comment

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

Breaks old rubygems where name tuples are not equivalent even when NameTuple#to_s is equal.

@martinemde
Copy link
Copy Markdown
Contributor

Ok, I have a few more minutes to look at this.

Original PR is #7081.

Useful to know: Gem::Platform also overrides self.new without problems.

If we didn't override new, we'd need to override initialize (which I had a lot of problems with when I first time, but we could try again) or override a few of the other instance methods.

We could also be better about removing the patch when it's not necessary. I'll test that approach by pushing to this branch.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

We could also be better about removing the patch when it's not necessary. I'll test that approach by pushing to this branch.

Yes, that's the approach we normally follow, feature detect if RubyGems is missing the feature/fix that needs patching (or hardcode the RubyGems version that first introduces it) and only apply it when necessary. I feel that also makes it easier to remember cleaning up when dropping support for old versions.

Also I think removing these patches should make some test fail? We test Bundler with old rubygems so some matrix entry should catch the issue?

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

Even if there's no end to end test that catches it, this is needed, so closing!

@deivid-rodriguez deivid-rodriguez deleted the debug-checksums branch December 11, 2023 19:47
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.

2 participants