titusfortner / webdrivers

Keep your Selenium WebDrivers updated automatically
MIT License
593 stars 111 forks source link

Drop `git ls-files` in gemspec #179

Closed utkarsh2102 closed 4 years ago

utkarsh2102 commented 4 years ago

Hi @kapoorlakshya (again! :smile:),

Thanks for working on this! :heart: However, while maintaining this in Debian, we found that this library relies on git to list the files which could be done via pure Ruby alternative -- which is what this PR does.

As an addition, this PR makes sure that this gem only ships the required files to the end-users and not other things which are not needed by them! :rocket:

Also, added rubocop-packaging as a development_dependency which will ensure the best practices. Here's what it showed us:

➜  webdrivers git:(master) ✗ rubocop --only Packaging

Inspecting 32 files
...............................C

Offenses:

webdrivers.gemspec:25:21: C: Packaging/GemspecGit: Avoid using git to produce lists of files.
Downstreams often need to build your package in an environment that does not have git (on purpose).
Use some pure Ruby alternative, like Dir or Dir.glob.
  s.files         = `git ls-files`.split("\n")
                    ^^^^^^^^^^^^^^
webdrivers.gemspec:26:21: C: Packaging/GemspecGit: Avoid using git to produce lists of files.
Downstreams often need to build your package in an environment that does not have git (on purpose).
Use some pure Ruby alternative, like Dir or Dir.glob.
  s.test_files    = `git ls-files -- {test,spec,features}/*`.split("\n")
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

32 files inspected, 2 offenses detected

And this PR fixes the same, which helps us in shipping this in Debian! :tada: Hope this would make sense and you'll be open to this change :100:

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>

utkarsh2102 commented 4 years ago

Hi @titusfortner,

The reason for git is so that it respects the gitignore file.

Fair enough. I have tweaked this PR so that it respects the .gitignore file. Hope that's better now? :smile:

Unless bundler has a new default? rubygems/bundler#5810

We (Debian core devs) are working with bundler/rubygems maintainers to work on this so that it doesn't use git by default! Both, Bundler and RubyGems, don't use git themselves. So hopefully soon, we'll be changing the defaults, too! :smile:

titusfortner commented 4 years ago

Hey sorry, I was communicating via phone last night and didn't get all of the context for this and what you are trying to do. My default is to do what the bundler authors provide by default since it feels weird to try to change things project by project against that de facto recommendation. That said, I see that both Capybara & Selenium avoid using git, so it makes sense for this project to drop it as a requirement as well.

Thank you for the contribution, and sorry for the extra back and forth.