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 #1183

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

utkarsh2102
Copy link
Contributor

Hi @iMacTia,

Thanks for your work on this!

Whilst maintaining this in Debian, we found that this library relies on git to produce a list of files. Using git in gemspec files in problematic, in general.

It also adds the Packaging extension of RuboCop.
More about it can be found on https://docs.rubocop.org/rubocop-packaging/

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

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Hi @utkarsh2102, thanks for taking care of faraday after doing the same for faraday_middleware.
I'd happily merge this change as well, but I noticed a require path was missed in the change.

faraday.gemspec Outdated Show resolved Hide resolved
@utkarsh2102
Copy link
Contributor Author

Hi @iMacTia,

I'd happily merge this change as well, but I noticed a require path was missed in the change.

Thanks, I missed that, apologies. But has been fixed already! 😄
Also, I'd want to do the same set of changes for faraday-http, hope you'd be open for that as well?

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Thanks @utkarsh2102 and sorry for the slow reply, I was travelling over the past days.
I'm still not 100% sure the current changes are compatible.
I'm particularly worried by comparing the old files list with the new one:

# OLD
files = %w[CHANGELOG.md LICENSE.md README.md Rakefile examples lib spec]

# NEW
files = Dir['CHANGELOG.md', '{examples,lib}/**/*', 'LICENSE.md', 'README.md']

I'm not sure the Rakefile and the spec folder are still included with the syntax change, but I may be wrong?

More than happy to have a similar PR for faraday-http as well ❤️ !

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Oh, I missed pressing "Submit Review". Dang.

faraday.gemspec Outdated Show resolved Hide resolved
faraday.gemspec Outdated Show resolved Hide resolved
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
@utkarsh2102
Copy link
Contributor Author

Hi @iMacTia 👋🏻

Thanks @utkarsh2102 and sorry for the slow reply, I was travelling over the past days.

No problem at all! Thanks for all the reviews, you're still very fast and kind! 💫

I'm still not 100% sure the current changes are compatible.
I'm particularly worried by comparing the old files list with the new one:

# OLD
files = %w[CHANGELOG.md LICENSE.md README.md Rakefile examples lib spec]

# NEW
files = Dir['CHANGELOG.md', '{examples,lib}/**/*', 'LICENSE.md', 'README.md']

I'm not sure the Rakefile and the spec folder are still included with the syntax change, but I may be wrong?

No, you're absolutely right! I didn't quite understand the relevance of shipping spec/ and that of pec/external_adapters, but thanks to @olleolleolle, I do now! 😅

More than happy to have a similar PR for faraday-http as well heart !

Great, thank you so much, again! ❤️

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Looks good now and all comments addressed 🎉 !
LGTM

@iMacTia iMacTia merged commit a5b7a6b into lostisland:master Sep 23, 2020
@utkarsh2102 utkarsh2102 deleted the drop-git-in-gemspec branch September 23, 2020 09:43
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