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

Move shebang to the top of bin/console template #3927

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

speckins
Copy link
Contributor

@speckins speckins commented Sep 3, 2020

In an executable script, the shebang line should be the first line (the
file needs to start with the bytes 0x23 0x21). Putting a comment above
it will break the script.

@welcome
Copy link

welcome bot commented Sep 3, 2020

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thank you, nice catch!

As illustrated by the failing specs, you need to restore the # frozen_string_literal: true line as the second line in the file.

@speckins
Copy link
Contributor Author

speckins commented Sep 3, 2020

I've made the requested changes.

@deivid-rodriguez
Copy link
Member

Thanks! Just one final request, could you add a couple of lines below this code here:

expect(bundled_app("#{gem_name}/bin/setup")).to be_executable
expect(bundled_app("#{gem_name}/bin/console")).to be_executable

to assert that the first two characters in the generated binstub are "!#"?

That way we add some test coverage for this bug fix.

In an executable script, the shebang line should be the first line (the
file needs to start with the bytes 0x23 0x21).  Putting a comment above
it will break the script.

(Regression test included per @deivid-rodriguez)
@speckins
Copy link
Contributor Author

speckins commented Sep 3, 2020

I appended those expectations to that same test rather than creating a separate test; I hope that's okay.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Yes, it looks really good, thanks!

@deivid-rodriguez deivid-rodriguez changed the title Remove comment above shebang in bin/console template Move shebang to the top of bin/console template Sep 3, 2020
@hsbt
Copy link
Member

hsbt commented Sep 4, 2020

👍

@hsbt hsbt merged commit aecc90b into rubygems:master Sep 4, 2020
@hsbt hsbt added this to the 3.2.0 milestone Sep 4, 2020
@speckins speckins deleted the patch-1 branch September 5, 2020 22:16
hsbt added a commit that referenced this pull request Sep 24, 2020
Move shebang to the top of `bin/console` template
deivid-rodriguez pushed a commit that referenced this pull request Oct 5, 2020
Move shebang to the top of `bin/console` template

(cherry picked from commit aecc90b)
deivid-rodriguez pushed a commit that referenced this pull request Oct 5, 2020
Move shebang to the top of `bin/console` template

(cherry picked from commit aecc90b)
deivid-rodriguez pushed a commit that referenced this pull request Oct 5, 2020
Move shebang to the top of `bin/console` template

(cherry picked from commit aecc90b)
deivid-rodriguez pushed a commit that referenced this pull request Oct 6, 2020
Move shebang to the top of `bin/console` template

(cherry picked from commit aecc90b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants