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 git ls-files in gemspec #179

Merged
merged 2 commits into from
Aug 13, 2020
Merged

Conversation

utkarsh2102
Copy link
Contributor

Hi @kapoorlakshya (again! 😄),

Thanks for working on this! ❤️
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! 🚀

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! 🎉
Hope this would make sense and you'll be open to this change 💯

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

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

@titusfortner titusfortner left a comment

Choose a reason for hiding this comment

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

The reason for git is so that it respects the gitignore file. Unless bundler has a new default? rubygems/bundler#5810

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
@utkarsh2102
Copy link
Contributor Author

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? 😄

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! 😄

@titusfortner
Copy link
Owner

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.

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