search

fakefs / fakefs

A fake filesystem. Use it in your tests.
MIT License
1.05k stars 188 forks source link

Explicitely depends on required default gems #494

Open Annih opened 3 months ago

Annih commented 3 months ago

While it is not required, as default gems are automatically added to the bundle, it could be considered as good practice to explictely add all actual dependencies.

Note: the reason why I decided to change that, is because I'm working in an environment where irb is not marked as a default gem (Ruby 3.1.0 on RockyLinux 8.9) and this forces me to specify irb in my app bundle if I want my app to pass CI tests.

⚠️ this is just a proposal, if you don't want it I understand :D

Annih commented 3 months ago

@grosser not sure I understand your message, what do you mean?

grosser commented 3 months ago

is this PR supposed to fix the issue you described ? ... because it does not look like it does. ... if it does then I'm happy to merge since it looks pretty innocent, but I don't understand how it solves the problem

Annih commented 3 months ago

Ahh yes I understand now :D

Sorry I temporarily reverted the addition of the dependency to check why the CI is failing, but it seems it was failing prior to my change (since this innocent patch is still not passing the CI).

I'll push again my actual change..

grosser commented 3 months ago

... could we maybe do require 'irb' and rescue LoadError instead ?

Annih commented 3 months ago

... could we maybe do require 'irb' and rescue LoadError instead ?

This would indeed fix my issue :)

Do you want me to split my PR?

  1. to rescue LoadError instead of adding the default gems deps
  2. to fix the "future" irb compat issue
  3. to fix the current ruby-2.7 CI issues about File
grosser commented 3 months ago

maybe just leave this one alone and make a new PR that just does the rescue

2.7 can be dropped from CI since it's EOL anyways