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

Specify utensils as a development dependency #117

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ollieh-m
Copy link

@ollieh-m ollieh-m commented Jul 20, 2022

What

Remove helpers/have_hash_matcher.rb and instead use the have_hash matcher defined in the utensils gem.

Why

As of this change, utensils includes a have_hash matcher which has a fix when have_hash is used in some js specs (see cookpad/utensils#1 for an explanation).

Currently, we could end up using the wrong version of have_hash in global-web if we require streamy/helpers/rspec_helper after requiring utensils/have_hash_matcher. This PR removes the divergent matcher - there's now only a single, correct have_hash matcher used in all contexts, so no risk of ending up with the wrong one 😅.

How

I've added utensils as a development dependency in the .gemspec. It's not possible to specify the github repo for the dependency in the .gemspec, so I've also added the gem to the regular gemfile - this means when running the gem locally, the cookpad/utensils version of the gem is used, which is the version that has have_hash.

I've googled around but can't see any way to specify a particular source for a gem as part of the gem's dependencies. Please let me know if you can see a way I'm missing 😄.

Anything else

The final step will be to update the streamy gem in global-web.

Use the have_hash matcher from utensils
Use `cookpad/utensils` (where have_hash is introduced) when running the gem
@ollieh-m ollieh-m marked this pull request as ready for review July 25, 2022 12:45
@Bodacious
Copy link

LGTM @ollieh-m

Any thoughts on publishing a forked version of the gem to Rubygems? (e.g. cookpad-utensils) That will give us more control over changes we make to the gem over time.

@balvig
Copy link
Contributor

balvig commented Aug 17, 2022

@ollieh-m @Bodacious Hello from the ghost of Christmas past! 👻

I suppose it's not a big deal either way, but just wanted to say that I'm happy to hand over the 🔑 to utensils including the gem on Rubygems.

As you can tell from my shoddy attempt at coming up with a cooking-related name 😅 , the gem was always intended for Cookpad usage and parity across Cookpad projects.

I think it probably ended up at balvig/utensils simply because that was easier to do at the time...? 🤦

So yeh, let me know what you prefer and we can set up something! 🙏

@Bodacious
Copy link

Hi @balvig! It's nice to bump into you here 👋

If you're happy to, I think it would be a good idea if we were able to have at least joint ownership of the gem, so that we didn't have to maintain our own separate fork. 🙇‍♂️

FWIW, I think the name is grand!

@balvig
Copy link
Contributor

balvig commented Aug 18, 2022

👍 let me know who to add as an owner (rubygems email/handle) and I'll set it up :)

@ollieh-m
Copy link
Author

ollieh-m commented Mar 2, 2023

🤦‍♂️ Well this fell off my radar.

👍 let me know who to add as an owner (rubygems email/handle) and I'll set it up :)

Could you add [email protected]?

And to clarify, will we be able to make the current fork (at cookpad/utensils) the utensils gem? We don't need to create a new gem right?

🙇‍♂️

@Bodacious
Copy link

And to clarify, will we be able to make the current fork (at cookpad/utensils) the utensils gem? We don't need to create a new gem right?

As long as you have write access on Rubygems, you should be able to publish new versions from the fork without having to cut a new gem 🤓

@balvig
Copy link
Contributor

balvig commented Mar 6, 2023

@ollieh-m you should be added now!

CleanShot 2023-03-06 at 12 58 23@2x

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.

4 participants