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

Drop base64 dependency, resolve Ruby warning #144

Merged
merged 1 commit into from
Sep 8, 2024

Conversation

Earlopain
Copy link
Contributor

base64 is being used in the gem and so just adding it to the Gemfile is not sufficient. Instead of moving it to the gemspec, just replace it through the equivalent pack method call.

@amatsuda
Copy link
Member

amatsuda commented Sep 8, 2024

base64 is being used in the gem and so just adding it to the Gemfile is not sufficient.

Right. I knew it, and intentionally made it so. Bacause for instance activesupport depends on base64, most of real-world apps would be automatically depending on base64 already. And where that's not the case, e.g.) where running Ruby 3.3+ on Rails 6.0 or lower for some reason, users can resolve this problem on their side by adding base64 to their app dependency. Of course I could add base64 to this gem's dependency, but I did not choose that path because I thought we cannot guarantee that base64 would keep supporting all Ruby versions that we're going to support here.

And regarding this patch, it's true that reducing a silent dependency is nice, however this change adds a little bit of cryptic taste to the code for the readers.

So there's such trade offs, and I think in this case we could improve the code readability by adding a comment. Thus, in conclusion, I'll merge this. Thank you @Earlopain!

@amatsuda amatsuda merged commit 45362b4 into simplecov-ruby:main Sep 8, 2024
11 checks passed
@Earlopain
Copy link
Contributor Author

Thanks for the quick merge!

It's true that activesupport depends on it but I don't think implicitly relying on it like that would be a good idea, there's no guarantee that they will keep it. For example, mutex_m was dropped after initially being added. Plus, I think there's plenty usage in gems without any ties to rails/activesupport (or other gems that themselves depend on base64).

@Earlopain Earlopain deleted the drop-base64 branch September 8, 2024 16:19
@koic
Copy link
Contributor

koic commented Sep 9, 2024

I noticed this through the following CI failure:
https://github.com/rubocop/rubocop/actions/runs/10765086770/job/29848821614?pr=13208

@amatsuda Do you have any release plans for the new version of simplecov-html?

@amatsuda
Copy link
Member

amatsuda commented Sep 9, 2024

OK, just made a release (0.13.1) with this patch since here came a real user who wants this fix. Thanks!

@koic
Copy link
Contributor

koic commented Sep 9, 2024

Great! Thanks!

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