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

Support magic comments in .ru files #1545

Merged
merged 1 commit into from Feb 2, 2020

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Feb 2, 2020

  • Such as # frozen_string_literal: true.

This seems actually like a relevant bug fix because many files under example and test use # frozen_string_literal: true but actually it has not effect before this fix.

From the idea in #1544 (comment)
cc @ioquatix @jeremyevans

* Such as # frozen_string_literal: true.
@ioquatix
Copy link
Member

ioquatix commented Feb 2, 2020

Another reason why we should have --warnings on by default during test runs.

@ioquatix ioquatix merged commit e3fd0c4 into rack:master Feb 2, 2020
@ioquatix
Copy link
Member

ioquatix commented Feb 2, 2020

Thanks for your awesome implementation! It's quite possibly the most awesome thing in Ruby I've seen in several months at least.

@eregon
Copy link
Contributor Author

eregon commented Feb 2, 2020

Thank you for the kind words :)

@shime
Copy link
Contributor

shime commented Feb 24, 2020

I'm writing a deep dive post about rackup and I have to say, this is the most amazing piece of code I've stumbled upon. Pure wizardry.

I don't fully understand why TOPLEVEL_BINDING is necessary here, though. What would be the effect of not having it?

# Why not something like this?
def self.new_from_string(builder_script, file = "(rackup)")
  bind, builder = Rack::Builder.new.instance_eval { [binding, self] }
  eval builder_script, bind, file
  builder.to_app
end

@eregon
Copy link
Contributor Author

eregon commented Feb 24, 2020

That would inherit the Module.nesting, which is [Rack::Builder, Rack] in that case. That would imply a .ru file with p Builder would print Rack::Builder and so defining your own Builder class would actually instead reopen Rack's Builder class which is most likely unintended.

TOPLEVEL_BINDING is used to preserve an empty Module.nesting just like regular top-level code in a file.

@eregon
Copy link
Contributor Author

eregon commented Feb 24, 2020

Some more details in my tweet: https://twitter.com/eregontp/status/1232037638485487617

@shime
Copy link
Contributor

shime commented Feb 25, 2020

Thank you! 🙏

dentarg added a commit to Starkast/wikimum that referenced this pull request Apr 19, 2020
Thanks to rack/rack#1545, no more warnings about
the magic comment

**Before**

    $ bundle e rake
    Run options: --seed 50920

    # Running:

    .................

    Finished in 0.046828s, 363.0307 runs/s, 405.7402 assertions/s.

    17 runs, 19 assertions, 0 failures, 0 errors, 0 skips
    config.ru:1: warning: `frozen_string_literal' is ignored after any tokens
    I, [2020-04-19T19:51:59.845036 #67031]  INFO -- sentry: ** [Raven] Raven 2.9.0 configured not to capture errors: DSN not set
    config.ru:1: warning: `frozen_string_literal' is ignored after any tokens
    Run options: --seed 40615

    # Running:

    ..........

    Finished in 1.222135s, 8.1824 runs/s, 10.6371 assertions/s.

    10 runs, 13 assertions, 0 failures, 0 errors, 0 skips
    Running RuboCop...
    Inspecting 48 files
    ................................................

    48 files inspected, no offenses detected

**After**

    $ bundle e rake
    Run options: --seed 63735

    # Running:

    .................

    Finished in 0.021696s, 783.5546 runs/s, 875.7375 assertions/s.

    17 runs, 19 assertions, 0 failures, 0 errors, 0 skips
    I, [2020-04-19T19:57:25.660098 #68534]  INFO -- sentry: ** [Raven] Raven 2.9.0 configured not to capture errors: DSN not set
    Run options: --seed 43133

    # Running:

    ..........

    Finished in 1.161220s, 8.6116 runs/s, 11.1951 assertions/s.

    10 runs, 13 assertions, 0 failures, 0 errors, 0 skips
    Running RuboCop...
    Inspecting 48 files
    ................................................

    48 files inspected, no offenses detected
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.

None yet

3 participants